From 51c6d5219f10a5a2b264e88181740b4ebb18fef1 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 20 May 2026 07:04:21 -0300 Subject: [PATCH] Fix config Sanitize fields missing from desanitize, causing FakeSetting to be persisted (#36619) * Add TestDesanitizeRemovesAllFakeSettings to catch future omissions Walks every string field in the config after a Sanitize+desanitize round-trip and fails if any still holds FakeSetting. This catches the case where a field is added to Sanitize without a corresponding desanitize entry. * Fix ElasticsearchSettings.ClientKey being incorrectly masked as a secret ClientKey is a file path, not a secret value. Masking it caused the asterisk string to be persisted to the database on config writes, which broke TLS client auth on restart. * Fix desanitize missing entries for fields added in 504fb96fdd98 504fb96fdd98 masked five fields in Sanitize without adding the corresponding desanitize entries, meaning a config save through the API would permanently overwrite those fields with FakeSetting: - FileSettings.ExportAmazonS3SecretAccessKey - ServiceSettings.GoogleDeveloperKey - ServiceSettings.GiphySdkKey - CacheSettings.RedisPassword - AutoTranslationSettings.LibreTranslate.APIKey * fixup! Add TestDesanitizeRemovesAllFakeSettings to catch future omissions * fixup! Fix desanitize missing entries for fields added in 504fb96fdd98 --------- Co-authored-by: Mattermost Build --- server/config/utils.go | 21 +++++++ server/config/utils_test.go | 94 ++++++++++++++++++++++++++++++ server/public/model/config.go | 4 -- server/public/model/config_test.go | 2 - 4 files changed, 115 insertions(+), 6 deletions(-) diff --git a/server/config/utils.go b/server/config/utils.go index 63f486fa68a..c2ecf25bb27 100644 --- a/server/config/utils.go +++ b/server/config/utils.go @@ -33,6 +33,9 @@ func desanitize(actual, target *model.Config) { if *target.FileSettings.AmazonS3SecretAccessKey == model.FakeSetting { target.FileSettings.AmazonS3SecretAccessKey = actual.FileSettings.AmazonS3SecretAccessKey } + if target.FileSettings.ExportAmazonS3SecretAccessKey != nil && *target.FileSettings.ExportAmazonS3SecretAccessKey == model.FakeSetting { + target.FileSettings.ExportAmazonS3SecretAccessKey = actual.FileSettings.ExportAmazonS3SecretAccessKey + } if target.FileSettings.AzureAccessKey != nil && *target.FileSettings.AzureAccessKey == model.FakeSetting { target.FileSettings.AzureAccessKey = actual.FileSettings.AzureAccessKey } @@ -95,6 +98,24 @@ func desanitize(actual, target *model.Config) { *target.ServiceSettings.SplitKey = *actual.ServiceSettings.SplitKey } + if target.ServiceSettings.GoogleDeveloperKey != nil && *target.ServiceSettings.GoogleDeveloperKey == model.FakeSetting { + target.ServiceSettings.GoogleDeveloperKey = actual.ServiceSettings.GoogleDeveloperKey + } + + if target.ServiceSettings.GiphySdkKey != nil && *target.ServiceSettings.GiphySdkKey == model.FakeSetting { + target.ServiceSettings.GiphySdkKey = actual.ServiceSettings.GiphySdkKey + } + + if target.CacheSettings.RedisPassword != nil && *target.CacheSettings.RedisPassword == model.FakeSetting { + target.CacheSettings.RedisPassword = actual.CacheSettings.RedisPassword + } + + if target.AutoTranslationSettings.LibreTranslate != nil && + target.AutoTranslationSettings.LibreTranslate.APIKey != nil && + *target.AutoTranslationSettings.LibreTranslate.APIKey == model.FakeSetting { + target.AutoTranslationSettings.LibreTranslate.APIKey = actual.AutoTranslationSettings.LibreTranslate.APIKey + } + for id, settings := range target.PluginSettings.Plugins { for k, v := range settings { if v == model.FakeSetting { diff --git a/server/config/utils_test.go b/server/config/utils_test.go index c7929ca6fa5..ddff6960021 100644 --- a/server/config/utils_test.go +++ b/server/config/utils_test.go @@ -4,6 +4,8 @@ package config import ( + "fmt" + "reflect" "testing" "github.com/stretchr/testify/assert" @@ -25,12 +27,15 @@ func TestDesanitize(t *testing.T) { actual.LdapSettings.BindPassword = new("bind_password") actual.FileSettings.PublicLinkSalt = new("public_link_salt") actual.FileSettings.AmazonS3SecretAccessKey = new("amazon_s3_secret_access_key") + actual.FileSettings.ExportAmazonS3SecretAccessKey = new("export_amazon_s3_secret_access_key") actual.EmailSettings.SMTPPassword = new("smtp_password") actual.GitLabSettings.Secret = new("secret") actual.OpenIdSettings.Secret = new("secret") actual.SqlSettings.DataSource = new("data_source") actual.SqlSettings.AtRestEncryptKey = new("at_rest_encrypt_key") actual.ElasticsearchSettings.Password = new("password") + actual.ServiceSettings.GoogleDeveloperKey = new("google_developer_key") + actual.ServiceSettings.GiphySdkKey = new("giphy_sdk_key") actual.SqlSettings.DataSourceReplicas = append(actual.SqlSettings.DataSourceReplicas, "replica0") actual.SqlSettings.DataSourceReplicas = append(actual.SqlSettings.DataSourceReplicas, "replica1") actual.SqlSettings.DataSourceSearchReplicas = append(actual.SqlSettings.DataSourceSearchReplicas, "search_replica0") @@ -53,12 +58,15 @@ func TestDesanitize(t *testing.T) { target.LdapSettings.BindPassword = model.NewPointer(model.FakeSetting) target.FileSettings.PublicLinkSalt = model.NewPointer(model.FakeSetting) target.FileSettings.AmazonS3SecretAccessKey = model.NewPointer(model.FakeSetting) + target.FileSettings.ExportAmazonS3SecretAccessKey = model.NewPointer(model.FakeSetting) target.EmailSettings.SMTPPassword = model.NewPointer(model.FakeSetting) target.GitLabSettings.Secret = model.NewPointer(model.FakeSetting) target.OpenIdSettings.Secret = model.NewPointer(model.FakeSetting) target.SqlSettings.DataSource = model.NewPointer(model.FakeSetting) target.SqlSettings.AtRestEncryptKey = model.NewPointer(model.FakeSetting) target.ElasticsearchSettings.Password = model.NewPointer(model.FakeSetting) + target.ServiceSettings.GoogleDeveloperKey = model.NewPointer(model.FakeSetting) + target.ServiceSettings.GiphySdkKey = model.NewPointer(model.FakeSetting) target.SqlSettings.DataSourceReplicas = []string{model.FakeSetting, model.FakeSetting} target.SqlSettings.DataSourceSearchReplicas = []string{model.FakeSetting, model.FakeSetting} target.PluginSettings.Plugins = map[string]map[string]any{ @@ -80,18 +88,104 @@ func TestDesanitize(t *testing.T) { assert.Equal(t, *actual.LdapSettings.BindPassword, *target.LdapSettings.BindPassword) assert.Equal(t, *actual.FileSettings.PublicLinkSalt, *target.FileSettings.PublicLinkSalt) assert.Equal(t, *actual.FileSettings.AmazonS3SecretAccessKey, *target.FileSettings.AmazonS3SecretAccessKey) + assert.Equal(t, *actual.FileSettings.ExportAmazonS3SecretAccessKey, *target.FileSettings.ExportAmazonS3SecretAccessKey) assert.Equal(t, *actual.EmailSettings.SMTPPassword, *target.EmailSettings.SMTPPassword) assert.Equal(t, *actual.GitLabSettings.Secret, *target.GitLabSettings.Secret) assert.Equal(t, *actual.OpenIdSettings.Secret, *target.OpenIdSettings.Secret) assert.Equal(t, *actual.SqlSettings.DataSource, *target.SqlSettings.DataSource) assert.Equal(t, *actual.SqlSettings.AtRestEncryptKey, *target.SqlSettings.AtRestEncryptKey) assert.Equal(t, *actual.ElasticsearchSettings.Password, *target.ElasticsearchSettings.Password) + assert.Equal(t, *actual.ServiceSettings.GoogleDeveloperKey, *target.ServiceSettings.GoogleDeveloperKey) + assert.Equal(t, *actual.ServiceSettings.GiphySdkKey, *target.ServiceSettings.GiphySdkKey) assert.Equal(t, actual.SqlSettings.DataSourceReplicas, target.SqlSettings.DataSourceReplicas) assert.Equal(t, actual.SqlSettings.DataSourceSearchReplicas, target.SqlSettings.DataSourceSearchReplicas) assert.Equal(t, actual.ServiceSettings.SplitKey, target.ServiceSettings.SplitKey) assert.Equal(t, actual.PluginSettings.Plugins, target.PluginSettings.Plugins) } +// TestDesanitizeRemovesAllFakeSettings verifies that every field masked by +// Sanitize has a corresponding entry in desanitize, so FakeSetting is never +// written back to stored config. No manual field listing is required: all +// string fields are pre-populated via reflection so Sanitize will mask any +// secret regardless of its default value. +func TestDesanitizeRemovesAllFakeSettings(t *testing.T) { + actual := &model.Config{} + actual.SetDefaults() + populateStrings(reflect.ValueOf(actual), "test-value") + + sanitized := actual.Clone() + sanitized.Sanitize(nil, nil) + + desanitize(actual, sanitized) + + assertNoFakeSettings(t, reflect.ValueOf(*sanitized), "Config") +} + +// populateStrings sets every empty string reachable from v to value so that +// Sanitize will replace it if it is a secret field. +func populateStrings(v reflect.Value, value string) { + switch v.Kind() { + case reflect.Pointer: + if v.IsNil() && v.CanSet() { + v.Set(reflect.New(v.Type().Elem())) + } + if !v.IsNil() { + if v.Elem().Kind() == reflect.String { + if v.Elem().String() == "" { + v.Elem().SetString(value) + } + } else { + populateStrings(v.Elem(), value) + } + } + case reflect.Struct: + for _, sf := range reflect.VisibleFields(v.Type()) { + field := v.FieldByIndex(sf.Index) + if field.CanSet() { + populateStrings(field, value) + } + } + case reflect.Slice: + for i := range v.Len() { + populateStrings(v.Index(i), value) + } + } +} + +// assertNoFakeSettings walks v recursively and fails if any string field equals +// model.FakeSetting, reporting the dotted path of the offending field. +func assertNoFakeSettings(t *testing.T, v reflect.Value, path string) { + t.Helper() + switch v.Kind() { + case reflect.Pointer: + if !v.IsNil() { + assertNoFakeSettings(t, v.Elem(), path) + } + case reflect.Struct: + for i := range v.NumField() { + assertNoFakeSettings(t, v.Field(i), path+"."+v.Type().Field(i).Name) + } + case reflect.String: + assert.NotEqual(t, model.FakeSetting, v.String(), "FakeSetting persisted at %s after desanitize", path) + case reflect.Slice: + for i := range v.Len() { + assertNoFakeSettings(t, v.Index(i), fmt.Sprintf("%s[%d]", path, i)) + } + case reflect.Map: + for _, key := range v.MapKeys() { + elem := v.MapIndex(key) + if elem.Kind() == reflect.Interface { + elem = elem.Elem() + } + assertNoFakeSettings(t, elem, fmt.Sprintf("%s[%v]", path, key)) + } + case reflect.Interface: + if !v.IsNil() { + assertNoFakeSettings(t, v.Elem(), path) + } + } +} + func TestFixInvalidLocales(t *testing.T) { // utils.TranslationsPreInit errors when TestFixInvalidLocales is run as part of testing the package, // but doesn't error when the test is run individually. diff --git a/server/public/model/config.go b/server/public/model/config.go index f001937219e..d975f59c46b 100644 --- a/server/public/model/config.go +++ b/server/public/model/config.go @@ -5263,10 +5263,6 @@ func (o *Config) Sanitize(pluginManifests []*Manifest, opts *SanitizeOptions) { *o.ElasticsearchSettings.Password = FakeSetting } - if o.ElasticsearchSettings.ClientKey != nil && *o.ElasticsearchSettings.ClientKey != "" { - *o.ElasticsearchSettings.ClientKey = FakeSetting - } - for i := range o.SqlSettings.DataSourceReplicas { o.SqlSettings.DataSourceReplicas[i] = sanitizeDataSourceField(o.SqlSettings.DataSourceReplicas[i], "SqlSettings.DataSourceReplicas") } diff --git a/server/public/model/config_test.go b/server/public/model/config_test.go index 96859087064..a0781e51a7d 100644 --- a/server/public/model/config_test.go +++ b/server/public/model/config_test.go @@ -1649,7 +1649,6 @@ func TestConfigSanitize(t *testing.T) { *c.OpenIdSettings.Secret = "secret" *c.ServiceSettings.GoogleDeveloperKey = "google-api-key" *c.ServiceSettings.GiphySdkKey = "giphy-sdk-key" - *c.ElasticsearchSettings.ClientKey = "/path/to/client-key.pem" *c.AutoTranslationSettings.LibreTranslate.APIKey = "libre-api-key" c.SqlSettings.DataSourceReplicas = []string{"stuff"} c.SqlSettings.DataSourceSearchReplicas = []string{"stuff"} @@ -1672,7 +1671,6 @@ func TestConfigSanitize(t *testing.T) { assert.Equal(t, FakeSetting, *c.SqlSettings.DataSource) assert.Equal(t, FakeSetting, *c.SqlSettings.AtRestEncryptKey) assert.Equal(t, FakeSetting, *c.ElasticsearchSettings.Password) - assert.Equal(t, FakeSetting, *c.ElasticsearchSettings.ClientKey) assert.Equal(t, FakeSetting, *c.ServiceSettings.GoogleDeveloperKey) assert.Equal(t, FakeSetting, *c.ServiceSettings.GiphySdkKey) assert.Equal(t, FakeSetting, c.SqlSettings.DataSourceReplicas[0])