[MM-57194] Allow plugins to mark setting fields as secret (#27986)

Co-authored-by: Claudio Costa <cstcld91@gmail.com>

server/channels/api4/config.go

@@ -205,7 +205,7 @@ func updateConfig(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 	auditRec.AddEventPriorState(&diffs)
 
-	newCfg.Sanitize()
+	c.App.SanitizedConfig(newCfg)
 
 	cfg, err = config.Merge(&model.Config{}, newCfg, &utils.MergeConfig{
 		StructFieldFilter: func(structField reflect.StructField, base, patch reflect.Value) bool {
@@ -355,7 +355,7 @@ func patchConfig(c *Context, w http.ResponseWriter, r *http.Request) {
 
 	auditRec.AddEventPriorState(&diffs)
 
-	newCfg.Sanitize()
+	c.App.SanitizedConfig(newCfg)
 
 	auditRec.Success()
 

server/channels/api4/config_local.go

@@ -77,7 +77,7 @@ func localUpdateConfig(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 	auditRec.AddEventPriorState(&diffs)
 
-	newCfg.Sanitize()
+	c.App.SanitizedConfig(newCfg)
 
 	auditRec.Success()
 	c.LogAudit("updateConfig")

server/channels/app/app_iface.go

@@ -320,6 +320,8 @@ type AppIface interface {
 	// RevokeSessionsFromAllUsers will go through all the sessions active
 	// in the server and revoke them
 	RevokeSessionsFromAllUsers() *model.AppError
+	// SanitizedConfig sanitizes a given configuration for a system admin without any secrets.
+	SanitizedConfig(cfg *model.Config)
 	// SaveConfig replaces the active configuration, optionally notifying cluster peers.
 	SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bool) (*model.Config, *model.Config, *model.AppError)
 	// SearchAllChannels returns a list of channels, the total count of the results of the search (if the paginate search option is true), and an error.

server/channels/app/config.go

@@ -214,11 +214,25 @@ func (a *App) GetConfigFile(name string) ([]byte, error) {
 // GetSanitizedConfig gets the configuration for a system admin without any secrets.
 func (a *App) GetSanitizedConfig() *model.Config {
 	cfg := a.Config().Clone()
-	cfg.Sanitize()
+
+	a.SanitizedConfig(cfg)
 
 	return cfg
 }
 
+// SanitizedConfig sanitizes a given configuration for a system admin without any secrets.
+func (a *App) SanitizedConfig(cfg *model.Config) {
+	manifests, err := a.getPluginManifests()
+	if err != nil {
+		// GetPluginManifests might error, e.g. when plugins are disabled.
+		// Sanitize all plugin settings in this case.
+		cfg.Sanitize(nil)
+		return
+	}
+
+	cfg.Sanitize(manifests)
+}
+
 // GetEnvironmentConfig returns a map of configuration keys whose values have been overridden by an environment variable.
 // If filter is not nil and returns false for a struct field, that field will be omitted.
 func (a *App) GetEnvironmentConfig(filter func(reflect.StructField) bool) map[string]any {

server/channels/app/opentracing/opentracing_layer.go

@@ -15239,6 +15239,21 @@ func (a *OpenTracingAppLayer) SanitizeTeams(session model.Session, teams []*mode
 	return resultVar0
 }
 
+func (a *OpenTracingAppLayer) SanitizedConfig(cfg *model.Config) {
+	origCtx := a.ctx
+	span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.SanitizedConfig")
+
+	a.ctx = newCtx
+	a.app.Srv().Store().SetContext(newCtx)
+	defer func() {
+		a.app.Srv().Store().SetContext(origCtx)
+		a.ctx = origCtx
+	}()
+
+	defer span.Finish()
+	a.app.SanitizedConfig(cfg)
+}
+
 func (a *OpenTracingAppLayer) SaveAcknowledgementForPost(c request.CTX, postID string, userID string) (*model.PostAcknowledgement, *model.AppError) {
 	origCtx := a.ctx
 	span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.SaveAcknowledgementForPost")

server/channels/app/plugin.go

@@ -378,6 +378,25 @@ func (ch *Channels) ShutDownPlugins() {
 	}
 }
 
+func (a *App) getPluginManifests() ([]*model.Manifest, error) {
+	pluginsEnvironment := a.GetPluginsEnvironment()
+	if pluginsEnvironment == nil {
+		return nil, model.NewAppError("GetPluginManifests", "app.plugin.disabled.app_error", nil, "", http.StatusNotImplemented)
+	}
+
+	plugins, err := pluginsEnvironment.Available()
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to get list of available plugins")
+	}
+
+	manifests := make([]*model.Manifest, len(plugins))
+	for i := range plugins {
+		manifests[i] = plugins[i].Manifest
+	}
+
+	return manifests, nil
+}
+
 func (a *App) GetActivePluginManifests() ([]*model.Manifest, *model.AppError) {
 	pluginsEnvironment := a.GetPluginsEnvironment()
 	if pluginsEnvironment == nil {

server/channels/app/plugin_api_test.go

@@ -813,43 +813,12 @@ func TestPluginAPISavePluginConfig(t *testing.T) {
 	assert.Equal(t, expectedConfiguration, savedConfiguration)
 }
 
-func TestPluginAPIGetPluginConfig(t *testing.T) {
-	th := Setup(t)
-	defer th.TearDown()
-
-	manifest := &model.Manifest{
-		Id: "pluginid",
-		SettingsSchema: &model.PluginSettingsSchema{
-			Settings: []*model.PluginSetting{
-				{Key: "MyStringSetting", Type: "text"},
-				{Key: "MyIntSetting", Type: "text"},
-				{Key: "MyBoolSetting", Type: "bool"},
-			},
-		},
-	}
-
-	api := NewPluginAPI(th.App, th.Context, manifest)
-
-	pluginConfigJsonString := `{"mystringsetting": "str", "myintsetting": 32, "myboolsetting": true}`
-	var pluginConfig map[string]any
-
-	err := json.Unmarshal([]byte(pluginConfigJsonString), &pluginConfig)
-	require.NoError(t, err)
-
-	th.App.UpdateConfig(func(cfg *model.Config) {
-		cfg.PluginSettings.Plugins["pluginid"] = pluginConfig
-	})
-
-	savedPluginConfig := api.GetPluginConfig()
-	assert.Equal(t, pluginConfig, savedPluginConfig)
-}
-
 func TestPluginAPILoadPluginConfiguration(t *testing.T) {
 	th := Setup(t)
 	defer th.TearDown()
 
 	var pluginJson map[string]any
-	err := json.Unmarshal([]byte(`{"mystringsetting": "str", "MyIntSetting": 32, "myboolsetting": true}`), &pluginJson)
+	err := json.Unmarshal([]byte(`{"mystringsetting": "str", "MyIntSetting": 32, "myBoolsetting": true}`), &pluginJson)
 	require.NoError(t, err)
 
 	th.App.UpdateConfig(func(cfg *model.Config) {

server/channels/app/plugin_api_tests/manual.test_load_configuration_plugin/main.go

@@ -34,6 +34,7 @@ func (p *MyPlugin) OnConfigurationChange() error {
 }
 
 func (p *MyPlugin) MessageWillBePosted(_ *plugin.Context, _ *model.Post) (*model.Post, string) {
+	// Test API.LoadPluginConfiguration
 	if p.configuration.MyStringSetting != "str" {
 		return nil, "MyStringSetting has invalid value"
 	}
@@ -43,6 +44,22 @@ func (p *MyPlugin) MessageWillBePosted(_ *plugin.Context, _ *model.Post) (*model
 	if !p.configuration.MyBoolSetting {
 		return nil, "MyBoolSetting has invalid value"
 	}
+
+	// Test API.GetPluginConfig
+	pc := p.API.GetPluginConfig()
+	if pc == nil {
+		return nil, "GetPluginConfig returned nil"
+	}
+	if pc["mystringsetting"] != "str" {
+		return nil, fmt.Sprintf("MyStringSetting has invalid value: %v", pc["mystringsetting"])
+	}
+	if pc["MyIntSetting"] != float64(32) {
+		return nil, fmt.Sprintf("MyIntSetting has invalid value: %v", pc["MyIntSetting"])
+	}
+	if pc["myBoolsetting"] != true {
+		return nil, fmt.Sprintf("MyBoolSetting has invalid value: %v", pc["myBoolsetting"])
+	}
+
 	return nil, "OK"
 }
 

server/config/diff.go

@@ -60,21 +60,24 @@ var configSensitivePaths = map[string]bool{
 // Sanitize replaces sensitive config values in the diff with asterisks filled strings.
 func (cd ConfigDiffs) Sanitize() ConfigDiffs {
 	if len(cd) == 1 {
+		// PluginSettings.Plugins gets sanitized anyway, so there is no need to use the plugin manifests here.
+		var pluginManifests []*model.Manifest
+
 		cfgPtr, ok := cd[0].BaseVal.(*model.Config)
 		if ok {
-			cfgPtr.Sanitize()
+			cfgPtr.Sanitize(pluginManifests)
 		}
 		cfgPtr, ok = cd[0].ActualVal.(*model.Config)
 		if ok {
-			cfgPtr.Sanitize()
+			cfgPtr.Sanitize(pluginManifests)
 		}
 		cfgVal, ok := cd[0].BaseVal.(model.Config)
 		if ok {
-			cfgVal.Sanitize()
+			cfgVal.Sanitize(pluginManifests)
 		}
 		cfgVal, ok = cd[0].ActualVal.(model.Config)
 		if ok {
-			cfgVal.Sanitize()
+			cfgVal.Sanitize(pluginManifests)
 		}
 	}
 

server/config/diff_test.go

@@ -113,7 +113,7 @@ func TestDiffSanitized(t *testing.T) {
 					Path: "",
 					BaseVal: func() model.Config {
 						cfg := defaultConfigGen()
-						cfg.Sanitize()
+						cfg.Sanitize(nil)
 						return *cfg
 					}(),
 					ActualVal: model.Config{},
@@ -131,7 +131,7 @@ func TestDiffSanitized(t *testing.T) {
 					BaseVal: model.Config{},
 					ActualVal: func() model.Config {
 						cfg := defaultConfigGen()
-						cfg.Sanitize()
+						cfg.Sanitize(nil)
 						return *cfg
 					}(),
 				},

server/public/model/config.go

@@ -3197,6 +3197,38 @@ func (s *PluginSettings) SetDefaults(ls LogSettings) {
 	}
 }
 
+// Sanitize cleans up the plugin settings by removing any sensitive information.
+// It does so by checking if the setting is marked as secret in the plugin manifest.
+// If it is, the setting is replaced with a fake value.
+// If a plugin is no longer installed, all settings of it's are sanitized.
+// If the list of manifests in nil, i.e. plugins are disabled, all settings are sanitized.
+func (s *PluginSettings) Sanitize(pluginManifests []*Manifest) {
+	manifestMap := make(map[string]*Manifest, len(pluginManifests))
+
+	for _, manifest := range pluginManifests {
+		manifestMap[manifest.Id] = manifest
+	}
+
+	for id, settings := range s.Plugins {
+		manifest := manifestMap[id]
+
+		for key := range settings {
+			if manifest == nil {
+				// Sanitize plugin settings for plugins that are not installed
+				settings[key] = FakeSetting
+				continue
+			}
+
+			for _, definedSetting := range manifest.SettingsSchema.Settings {
+				if definedSetting.Secret && strings.EqualFold(definedSetting.Key, key) {
+					settings[key] = FakeSetting
+					break
+				}
+			}
+		}
+	}
+}
+
 type WranglerSettings struct {
 	PermittedWranglerRoles                   []string
 	AllowedEmailDomain                       []string
@@ -4396,7 +4428,7 @@ func (o *Config) GetSanitizeOptions() map[string]bool {
 	return options
 }
 
-func (o *Config) Sanitize() {
+func (o *Config) Sanitize(pluginManifests []*Manifest) {
 	if o.LdapSettings.BindPassword != nil && *o.LdapSettings.BindPassword != "" {
 		*o.LdapSettings.BindPassword = FakeSetting
 	}
@@ -4462,6 +4494,8 @@ func (o *Config) Sanitize() {
 	if o.ServiceSettings.SplitKey != nil {
 		*o.ServiceSettings.SplitKey = FakeSetting
 	}
+
+	o.PluginSettings.Sanitize(pluginManifests)
 }
 
 // structToMapFilteredByTag converts a struct into a map removing those fields that has the tag passed

server/public/model/config_test.go

@@ -1387,7 +1387,7 @@ func TestConfigSanitize(t *testing.T) {
 		QueryTimeLag:     NewPointer("QueryTimeLag"),
 	}}
 
-	c.Sanitize()
+	c.Sanitize(nil)
 
 	assert.Equal(t, FakeSetting, *c.LdapSettings.BindPassword)
 	assert.Equal(t, FakeSetting, *c.FileSettings.PublicLinkSalt)
@@ -1409,12 +1409,201 @@ func TestConfigSanitize(t *testing.T) {
 	t.Run("with default config", func(t *testing.T) {
 		c := Config{}
 		c.SetDefaults()
-		c.Sanitize()
+		c.Sanitize(nil)
 
 		assert.Len(t, c.SqlSettings.ReplicaLagSettings, 0)
 	})
 }
 
+func TestPluginSettingsSanitize(t *testing.T) {
+	plugins := map[string]map[string]any{
+		"plugin.id": {
+			"somesetting":  "some value",
+			"secrettext":   "a secret",
+			"secretnumber": 123,
+		},
+		"another.plugin": {
+			"somesetting": 456,
+		},
+	}
+
+	for name, tc := range map[string]struct {
+		manifests []*Manifest
+		expected  map[string]map[string]any
+	}{
+		"nil list of manifests": {
+			manifests: nil,
+			expected: map[string]map[string]any{
+				"plugin.id": {
+					"somesetting":  FakeSetting,
+					"secrettext":   FakeSetting,
+					"secretnumber": FakeSetting,
+				},
+				"another.plugin": {
+					"somesetting": FakeSetting,
+				},
+			},
+		},
+		"empty list of manifests": {
+			manifests: []*Manifest{},
+			expected: map[string]map[string]any{
+				"plugin.id": {
+					"somesetting":  FakeSetting,
+					"secrettext":   FakeSetting,
+					"secretnumber": FakeSetting,
+				},
+				"another.plugin": {
+					"somesetting": FakeSetting,
+				},
+			},
+		},
+		"one plugin installed": {
+			manifests: []*Manifest{
+				{
+					Id: "plugin.id",
+					SettingsSchema: &PluginSettingsSchema{
+						Settings: []*PluginSetting{
+							{
+								Key:    "somesetting",
+								Type:   "text",
+								Secret: false,
+							},
+							{
+								Key:    "secrettext",
+								Type:   "text",
+								Secret: true,
+							},
+							{
+								Key:    "secretnumber",
+								Type:   "number",
+								Secret: true,
+							},
+						},
+					},
+				},
+			},
+			expected: map[string]map[string]any{
+				"plugin.id": {
+					"somesetting":  "some value",
+					"secrettext":   FakeSetting,
+					"secretnumber": FakeSetting,
+				},
+				"another.plugin": {
+					"somesetting": FakeSetting,
+				},
+			},
+		},
+		"two plugins installed": {
+			manifests: []*Manifest{
+				{
+					Id: "plugin.id",
+					SettingsSchema: &PluginSettingsSchema{
+						Settings: []*PluginSetting{
+							{
+								Key:    "somesetting",
+								Type:   "text",
+								Secret: false,
+							},
+							{
+								Key:    "secrettext",
+								Type:   "text",
+								Secret: true,
+							},
+							{
+								Key:    "secretnumber",
+								Type:   "number",
+								Secret: true,
+							},
+						},
+					},
+				},
+				{
+					Id: "another.plugin",
+					SettingsSchema: &PluginSettingsSchema{
+						Settings: []*PluginSetting{
+							{
+								Key:    "somesetting",
+								Type:   "number",
+								Secret: false,
+							},
+						},
+					},
+				},
+			},
+			expected: map[string]map[string]any{
+				"plugin.id": {
+					"somesetting":  "some value",
+					"secrettext":   FakeSetting,
+					"secretnumber": FakeSetting,
+				},
+				"another.plugin": {
+					"somesetting": 456,
+				},
+			},
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			name := name // TODO: Remove once go1.22 is used
+			tc := tc     // TODO: Remove once go1.22 is used
+
+			if name != "one plugin installed" {
+				return
+			}
+
+			c := PluginSettings{}
+			c.SetDefaults(*NewLogSettings())
+			c.Plugins = plugins
+
+			c.Sanitize(tc.manifests)
+
+			assert.Equal(t, tc.expected, c.Plugins, name)
+		})
+	}
+
+	t.Run("one plugin installed, two in the config", func(t *testing.T) {
+		c := PluginSettings{}
+		c.SetDefaults(*NewLogSettings())
+		c.Plugins = plugins
+
+		c.Sanitize([]*Manifest{
+			{
+				Id: "plugin.id",
+				SettingsSchema: &PluginSettingsSchema{
+					Settings: []*PluginSetting{
+						{
+							Key:    "somesetting",
+							Type:   "text",
+							Secret: false,
+						},
+						{
+							Key:    "secrettext",
+							Type:   "text",
+							Secret: true,
+						},
+						{
+							Key:    "secretnumber",
+							Type:   "number",
+							Secret: true,
+						},
+					},
+				},
+			},
+		})
+
+		expected := map[string]map[string]any{
+			"plugin.id": {
+				"somesetting":  "some value",
+				"secrettext":   FakeSetting,
+				"secretnumber": FakeSetting,
+			},
+			"another.plugin": {
+				"somesetting": FakeSetting,
+			},
+		}
+		assert.Equal(t, expected, c.Plugins)
+	})
+}
+
 func TestConfigFilteredByTag(t *testing.T) {
 	c := Config{}
 	c.SetDefaults()

server/public/model/manifest.go

@@ -89,6 +89,10 @@ type PluginSetting struct {
 	// and the opposite environment is running the plugin, the setting will be hidden in the admin console UI.
 	// Note that this functionality is entirely client-side, so the plugin needs to handle the case of invalid submissions.
 	Hosting string `json:"hosting"`
+
+	// If true, the setting is sanitized before showing it in the System Console or returning it via the API.
+	// This is useful for settings that contain sensitive information.
+	Secret bool `json:"secret"`
 }
 
 type PluginSettingsSection struct {