Fix config Sanitize fields missing from desanitize, causing FakeSetting to be persisted (#36619) (#36653)

* 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 504fb96fdd

504fb96fdd 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 504fb96fdd

---------

Co-authored-by: Mattermost Build <build@mattermost.com>

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.EmailSettings.SMTPPassword == model.FakeSetting {
 		target.EmailSettings.SMTPPassword = actual.EmailSettings.SMTPPassword
@@ -89,6 +92,18 @@ 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
+	}
+
 	for id, settings := range target.PluginSettings.Plugins {
 		for k, v := range settings {
 			if v == model.FakeSetting {

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 = model.NewPointer("bind_password")
 	actual.FileSettings.PublicLinkSalt = model.NewPointer("public_link_salt")
 	actual.FileSettings.AmazonS3SecretAccessKey = model.NewPointer("amazon_s3_secret_access_key")
+	actual.FileSettings.ExportAmazonS3SecretAccessKey = model.NewPointer("export_amazon_s3_secret_access_key")
 	actual.EmailSettings.SMTPPassword = model.NewPointer("smtp_password")
 	actual.GitLabSettings.Secret = model.NewPointer("secret")
 	actual.OpenIdSettings.Secret = model.NewPointer("secret")
 	actual.SqlSettings.DataSource = model.NewPointer("data_source")
 	actual.SqlSettings.AtRestEncryptKey = model.NewPointer("at_rest_encrypt_key")
 	actual.ElasticsearchSettings.Password = model.NewPointer("password")
+	actual.ServiceSettings.GoogleDeveloperKey = model.NewPointer("google_developer_key")
+	actual.ServiceSettings.GiphySdkKey = model.NewPointer("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.

server/public/model/config.go

@@ -4884,10 +4884,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")
 	}

server/public/model/config_test.go

@@ -1519,7 +1519,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.SqlSettings.DataSourceReplicas = []string{"stuff"}
 	c.SqlSettings.DataSourceSearchReplicas = []string{"stuff"}
 	c.SqlSettings.ReplicaLagSettings = []*ReplicaLagSettings{{
@@ -1540,7 +1539,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])