MM-8622: improved plugin error handling (#8692)

* don't report an error on plugin activation if already active

* improved plugin logging events

Log an error when a plugin's ServeHTTP fails, or when it unexpectedly
terminates.

Restart a plugin at most three times, allowing its failure to later
bubble up under the "failed to stay running" status.

* clarified plugin activation/deactivation

Avoid repeatedly activating when any configuration bit changes. Improved
logging.

* constrain plugin ids to ^[a-zA-Z0-9-_\.]+$ and enforce minimum length

Previously, the plugin id was used unsanitized to relocate the plugin
bundle, which allowed writing outside the `plugins/` directory by using
an `id` containing `../`.

Similarly, an empty string was accepted as an id and led to unexpected
error messages.

* remove plugins by manifest path, not id

If the id within the manifest ever diverges from the actual plugin
location, it becomes impossible to remove via the API. Instead, if the
plugin is found by id, remove the path containing the manifest.

* ignore plugins with nil manifests

If a plugin was detected, but had a manifest that couldn't be parsed, it
will be left nil but still be listed among the packages. Skip over these
in most cases to avoid segfaults.

* leverage mlog more effectively for plugins

* build issues

15 files changed, 256 insertions, 57 deletions

diff --git a/app/plugin.go b/app/plugin.go
index 3da9cea40..0aaa8d1d4 100644
--- a/app/plugin.go
+++ b/app/plugin.go
@@ -15,7 +15,6 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-	"unicode/utf8"
 
 	"github.com/gorilla/mux"
 	"github.com/mattermost/mattermost-server/mlog"
@@ -33,10 +32,6 @@ import (
 	"github.com/mattermost/mattermost-server/plugin/rpcplugin/sandbox"
 )
 
-const (
-	PLUGIN_MAX_ID_LENGTH = 190
-)
-
 var prepackagedPlugins map[string]func(string) ([]byte, error) = map[string]func(string) ([]byte, error){
 	"jira": jira.Asset,
 	"zoom": zoom.Asset,
@@ -47,7 +42,7 @@ func (a *App) initBuiltInPlugins() {
 		"ldapextras": &ldapextras.Plugin{},
 	}
 	for id, p := range plugins {
-		mlog.Debug("Initializing built-in plugin: " + id)
+		mlog.Debug("Initializing built-in plugin", mlog.String("plugin_id", id))
 		api := &BuiltInPluginAPI{
 			id:     id,
 			router: a.Srv.Router.PathPrefix("/plugins/" + id).Subrouter(),
@@ -65,21 +60,23 @@ func (a *App) initBuiltInPlugins() {
 	}
 }
 
-// ActivatePlugins will activate any plugins enabled in the config
-// and deactivate all other plugins.
-func (a *App) ActivatePlugins() {
+func (a *App) setPluginsActive(activate bool) {
 	if a.PluginEnv == nil {
-		mlog.Error("plugin env not initialized")
+		mlog.Error(fmt.Sprintf("Cannot setPluginsActive(%t): plugin env not initialized", activate))
 		return
 	}
 
 	plugins, err := a.PluginEnv.Plugins()
 	if err != nil {
-		mlog.Error("failed to activate plugins: " + err.Error())
+		mlog.Error(fmt.Sprintf("Cannot setPluginsActive(%t)", activate), mlog.Err(err))
 		return
 	}
 
 	for _, plugin := range plugins {
+		if plugin.Manifest == nil {
+			continue
+		}
+
 		id := plugin.Manifest.Id
 
 		pluginState := &model.PluginState{Enable: false}
@@ -89,15 +86,14 @@ func (a *App) ActivatePlugins() {
 
 		active := a.PluginEnv.IsPluginActive(id)
 
-		if pluginState.Enable && !active {
+		if activate && pluginState.Enable && !active {
 			if err := a.activatePlugin(plugin.Manifest); err != nil {
-				mlog.Error(fmt.Sprintf("%v plugin enabled in config.json but failing to activate err=%v", plugin.Manifest.Id, err.DetailedError))
-				continue
+				mlog.Error("Plugin failed to activate", mlog.String("plugin_id", plugin.Manifest.Id), mlog.String("err", err.DetailedError))
 			}
 
-		} else if !pluginState.Enable && active {
+		} else if (!activate || !pluginState.Enable) && active {
 			if err := a.deactivatePlugin(plugin.Manifest); err != nil {
-				mlog.Error(err.Error())
+				mlog.Error("Plugin failed to deactivate", mlog.String("plugin_id", plugin.Manifest.Id), mlog.String("err", err.DetailedError))
 			}
 		}
 	}
@@ -114,13 +110,13 @@ func (a *App) activatePlugin(manifest *model.Manifest) *model.AppError {
 		a.Publish(message)
 	}
 
-	mlog.Info(fmt.Sprintf("Activated %v plugin", manifest.Id))
+	mlog.Info("Activated plugin", mlog.String("plugin_id", manifest.Id))
 	return nil
 }
 
 func (a *App) deactivatePlugin(manifest *model.Manifest) *model.AppError {
 	if err := a.PluginEnv.DeactivatePlugin(manifest.Id); err != nil {
-		return model.NewAppError("removePlugin", "app.plugin.deactivate.app_error", nil, err.Error(), http.StatusBadRequest)
+		return model.NewAppError("deactivatePlugin", "app.plugin.deactivate.app_error", nil, err.Error(), http.StatusBadRequest)
 	}
 
 	a.UnregisterPluginCommands(manifest.Id)
@@ -131,7 +127,7 @@ func (a *App) deactivatePlugin(manifest *model.Manifest) *model.AppError {
 		a.Publish(message)
 	}
 
-	mlog.Info(fmt.Sprintf("Deactivated %v plugin", manifest.Id))
+	mlog.Info("Deactivated plugin", mlog.String("plugin_id", manifest.Id))
 	return nil
 }
 
@@ -174,8 +170,8 @@ func (a *App) installPlugin(pluginFile io.Reader, allowPrepackaged bool) (*model
 		return nil, model.NewAppError("installPlugin", "app.plugin.prepackaged.app_error", nil, "", http.StatusBadRequest)
 	}
 
-	if utf8.RuneCountInString(manifest.Id) > PLUGIN_MAX_ID_LENGTH {
-		return nil, model.NewAppError("installPlugin", "app.plugin.id_length.app_error", map[string]interface{}{"Max": PLUGIN_MAX_ID_LENGTH}, err.Error(), http.StatusBadRequest)
+	if !plugin.IsValidId(manifest.Id) {
+		return nil, model.NewAppError("installPlugin", "app.plugin.invalid_id.app_error", map[string]interface{}{"Min": plugin.MinIdLength, "Max": plugin.MaxIdLength, "Regex": plugin.ValidId.String()}, "", http.StatusBadRequest)
 	}
 
 	bundles, err := a.PluginEnv.Plugins()
@@ -184,7 +180,7 @@ func (a *App) installPlugin(pluginFile io.Reader, allowPrepackaged bool) (*model
 	}
 
 	for _, bundle := range bundles {
-		if bundle.Manifest.Id == manifest.Id {
+		if bundle.Manifest != nil && bundle.Manifest.Id == manifest.Id {
 			return nil, model.NewAppError("installPlugin", "app.plugin.install_id.app_error", nil, "", http.StatusBadRequest)
 		}
 	}
@@ -211,6 +207,10 @@ func (a *App) GetPlugins() (*model.PluginsResponse, *model.AppError) {
 
 	resp := &model.PluginsResponse{Active: []*model.PluginInfo{}, Inactive: []*model.PluginInfo{}}
 	for _, plugin := range plugins {
+		if plugin.Manifest == nil {
+			continue
+		}
+
 		info := &model.PluginInfo{
 			Manifest: *plugin.Manifest,
 		}
@@ -259,9 +259,11 @@ func (a *App) removePlugin(id string, allowPrepackaged bool) *model.AppError {
 	}
 
 	var manifest *model.Manifest
+	var pluginPath string
 	for _, p := range plugins {
-		if p.Manifest.Id == id {
+		if p.Manifest != nil && p.Manifest.Id == id {
 			manifest = p.Manifest
+			pluginPath = filepath.Dir(p.ManifestPath)
 			break
 		}
 	}
@@ -277,7 +279,7 @@ func (a *App) removePlugin(id string, allowPrepackaged bool) *model.AppError {
 		}
 	}
 
-	err = os.RemoveAll(filepath.Join(a.PluginEnv.SearchPath(), id))
+	err = os.RemoveAll(pluginPath)
 	if err != nil {
 		return model.NewAppError("removePlugin", "app.plugin.remove.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
@@ -372,12 +374,12 @@ func (a *App) InitPlugins(pluginPath, webappPath string, supervisorOverride plug
 	mlog.Info("Starting up plugins")
 
 	if err := os.Mkdir(pluginPath, 0744); err != nil && !os.IsExist(err) {
-		mlog.Error("failed to start up plugins: " + err.Error())
+		mlog.Error("Failed to start up plugins", mlog.Err(err))
 		return
 	}
 
 	if err := os.Mkdir(webappPath, 0744); err != nil && !os.IsExist(err) {
-		mlog.Error("failed to start up plugins: " + err.Error())
+		mlog.Error("Failed to start up plugins", mlog.Err(err))
 		return
 	}
 
@@ -399,15 +401,14 @@ func (a *App) InitPlugins(pluginPath, webappPath string, supervisorOverride plug
 	if supervisorOverride != nil {
 		options = append(options, pluginenv.SupervisorProvider(supervisorOverride))
 	} else if err := sandbox.CheckSupport(); err != nil {
-		mlog.Warn(err.Error())
-		mlog.Warn("plugin sandboxing is not supported. plugins will run with the same access level as the server. See documentation to learn more: https://developers.mattermost.com/extend/plugins/security/")
+		mlog.Warn("plugin sandboxing is not supported. plugins will run with the same access level as the server. See documentation to learn more: https://developers.mattermost.com/extend/plugins/security/", mlog.Err(err))
 		options = append(options, pluginenv.SupervisorProvider(rpcplugin.SupervisorProvider))
 	} else {
 		options = append(options, pluginenv.SupervisorProvider(sandbox.SupervisorProvider))
 	}
 
 	if env, err := pluginenv.New(options...); err != nil {
-		mlog.Error("failed to start up plugins: " + err.Error())
+		mlog.Error("Failed to start up plugins", mlog.Err(err))
 		return
 	} else {
 		a.PluginEnv = env
@@ -415,36 +416,34 @@ func (a *App) InitPlugins(pluginPath, webappPath string, supervisorOverride plug
 
 	for id, asset := range prepackagedPlugins {
 		if tarball, err := asset("plugin.tar.gz"); err != nil {
-			mlog.Error("failed to install prepackaged plugin: " + err.Error())
+			mlog.Error("Failed to install prepackaged plugin", mlog.Err(err))
 		} else if tarball != nil {
 			a.removePlugin(id, true)
 			if _, err := a.installPlugin(bytes.NewReader(tarball), true); err != nil {
-				mlog.Error("failed to install prepackaged plugin: " + err.Error())
+				mlog.Error("Failed to install prepackaged plugin", mlog.Err(err))
 			}
 			if _, ok := a.Config().PluginSettings.PluginStates[id]; !ok && id != "zoom" {
 				if err := a.EnablePlugin(id); err != nil {
-					mlog.Error("failed to enable prepackaged plugin: " + err.Error())
+					mlog.Error("Failed to enable prepackaged plugin", mlog.Err(err))
 				}
 			}
 		}
 	}
 
 	a.RemoveConfigListener(a.PluginConfigListenerId)
-	a.PluginConfigListenerId = a.AddConfigListener(func(prevCfg, cfg *model.Config) {
+	a.PluginConfigListenerId = a.AddConfigListener(func(_, cfg *model.Config) {
 		if a.PluginEnv == nil {
 			return
 		}
 
-		if *prevCfg.PluginSettings.Enable && *cfg.PluginSettings.Enable {
-			a.ActivatePlugins()
-		}
+		a.setPluginsActive(*cfg.PluginSettings.Enable)
 
 		for _, err := range a.PluginEnv.Hooks().OnConfigurationChange() {
 			mlog.Error(err.Error())
 		}
 	})
 
-	a.ActivatePlugins()
+	a.setPluginsActive(true)
 }
 
 func (a *App) ServePluginRequest(w http.ResponseWriter, r *http.Request) {
diff --git a/i18n/en.json b/i18n/en.json
index 0afaa326c..d4a08b07a 100644
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -3812,7 +3812,7 @@
   },
   {
     "id": "app.plugin.activate.app_error",
-    "translation": "Unable to activate extracted plugin. Plugin may already exist and be activated."
+    "translation": "Unable to activate extracted plugin."
   },
   {
     "id": "app.plugin.cluster.save_config.app_error",
@@ -3847,8 +3847,8 @@
     "translation": "Unable to get active plugins"
   },
   {
-    "id": "app.plugin.id_length.app_error",
-    "translation": "Plugin Id must be less than {{.Max}} characters."
+    "id": "app.plugin.invalid_id.app_error",
+    "translation": "Plugin Id must be at least {{.Min}} characters, at most {{.Max}} characters and match {{.Regex}}."
   },
   {
     "id": "app.plugin.install.app_error",
diff --git a/mlog/log.go b/mlog/log.go
index 22db0c7b1..ad537a11d 100644
--- a/mlog/log.go
+++ b/mlog/log.go
@@ -29,6 +29,7 @@ type Field = zapcore.Field
 var Int64 = zap.Int64
 var Int = zap.Int
 var String = zap.String
+var Err = zap.Error
 
 type LoggerConfiguration struct {
 	EnableConsole bool
diff --git a/model/manifest.go b/model/manifest.go
index 32d4341cd..d6a064d4e 100644
--- a/model/manifest.go
+++ b/model/manifest.go
@@ -93,9 +93,9 @@ type PluginSettingsSchema struct {
 //               help_text: When true, an extra thing will be enabled!
 //               default: false
 type Manifest struct {
-	// The id is a globally unique identifier that represents your plugin. Ids are limited
-	// to 190 characters. Reverse-DNS notation using a name you control is a good option.
-	// For example, "com.mycompany.myplugin".
+	// The id is a globally unique identifier that represents your plugin. Ids must be at least
+	// 3 characters, at most 190 characters and must match ^[a-zA-Z0-9-_\.]+$.
+	// Reverse-DNS notation using a name you control is a good option, e.g. "com.mycompany.myplugin".
 	Id string `json:"id" yaml:"id"`
 
 	// The name to be displayed for the plugin.
diff --git a/plugin/pluginenv/environment.go b/plugin/pluginenv/environment.go
index adc9ddbde..adc02e885 100644
--- a/plugin/pluginenv/environment.go
+++ b/plugin/pluginenv/environment.go
@@ -112,8 +112,12 @@ func (env *Environment) ActivatePlugin(id string) error {
 	env.mutex.Lock()
 	defer env.mutex.Unlock()
 
+	if !plugin.IsValidId(id) {
+		return fmt.Errorf("invalid plugin id: %s", id)
+	}
+
 	if _, ok := env.activePlugins[id]; ok {
-		return fmt.Errorf("plugin already active: %v", id)
+		return nil
 	}
 	plugins, err := ScanSearchPath(env.searchPath)
 	if err != nil {
diff --git a/plugin/pluginenv/environment_test.go b/plugin/pluginenv/environment_test.go
index 2a52b3830..91d639f69 100644
--- a/plugin/pluginenv/environment_test.go
+++ b/plugin/pluginenv/environment_test.go
@@ -149,7 +149,7 @@ func TestEnvironment(t *testing.T) {
 	assert.Equal(t, env.ActivePluginIds(), []string{"foo"})
 	activePlugins = env.ActivePlugins()
 	assert.Len(t, activePlugins, 1)
-	assert.Error(t, env.ActivatePlugin("foo"))
+	assert.NoError(t, env.ActivatePlugin("foo"))
 	assert.True(t, env.IsPluginActive("foo"))
 
 	hooks.On("OnDeactivate").Return(nil)
diff --git a/plugin/rpcplugin/hooks.go b/plugin/rpcplugin/hooks.go
index 7b44d0de7..90734fd1c 100644
--- a/plugin/rpcplugin/hooks.go
+++ b/plugin/rpcplugin/hooks.go
@@ -11,6 +11,7 @@ import (
 	"net/rpc"
 	"reflect"
 
+	"github.com/mattermost/mattermost-server/mlog"
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/plugin"
 )
@@ -165,6 +166,7 @@ type RemoteHooks struct {
 	muxer       *Muxer
 	apiCloser   io.Closer
 	implemented [maxRemoteHookCount]bool
+	pluginId    string
 }
 
 var _ plugin.Hooks = (*RemoteHooks)(nil)
@@ -237,6 +239,7 @@ func (h *RemoteHooks) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		Request:              forwardedRequest,
 		RequestBodyStream:    requestBodyStream,
 	}, nil); err != nil {
+		mlog.Error("Plugin failed to ServeHTTP", mlog.String("plugin_id", h.pluginId), mlog.Err(err))
 		http.Error(w, "500 internal server error", http.StatusInternalServerError)
 	}
 }
@@ -260,10 +263,11 @@ func (h *RemoteHooks) Close() error {
 	return h.client.Close()
 }
 
-func ConnectHooks(conn io.ReadWriteCloser, muxer *Muxer) (*RemoteHooks, error) {
+func ConnectHooks(conn io.ReadWriteCloser, muxer *Muxer, pluginId string) (*RemoteHooks, error) {
 	remote := &RemoteHooks{
-		client: rpc.NewClient(conn),
-		muxer:  muxer,
+		client:   rpc.NewClient(conn),
+		muxer:    muxer,
+		pluginId: pluginId,
 	}
 	implemented, err := remote.Implemented()
 	if err != nil {
diff --git a/plugin/rpcplugin/hooks_test.go b/plugin/rpcplugin/hooks_test.go
index 116038dae..c404442b7 100644
--- a/plugin/rpcplugin/hooks_test.go
+++ b/plugin/rpcplugin/hooks_test.go
@@ -31,7 +31,7 @@ func testHooksRPC(hooks interface{}, f func(*RemoteHooks)) error {
 	id, server := c1.Serve()
 	go ServeHooks(hooks, server, c1)
 
-	remote, err := ConnectHooks(c2.Connect(id), c2)
+	remote, err := ConnectHooks(c2.Connect(id), c2, "plugin_id")
 	if err != nil {
 		return err
 	}
diff --git a/plugin/rpcplugin/main.go b/plugin/rpcplugin/main.go
index 96a61c068..efb880605 100644
--- a/plugin/rpcplugin/main.go
+++ b/plugin/rpcplugin/main.go
@@ -30,7 +30,7 @@ func Main(hooks interface{}) {
 }
 
 // Returns the hooks being served by a call to Main.
-func ConnectMain(muxer *Muxer) (*RemoteHooks, error) {
+func ConnectMain(muxer *Muxer, pluginId string) (*RemoteHooks, error) {
 	buf := make([]byte, 1)
 	if _, err := muxer.Read(buf); err != nil {
 		return nil, err
@@ -43,5 +43,5 @@ func ConnectMain(muxer *Muxer) (*RemoteHooks, error) {
 		return nil, err
 	}
 
-	return ConnectHooks(muxer.Connect(id), muxer)
+	return ConnectHooks(muxer.Connect(id), muxer, pluginId)
 }
diff --git a/plugin/rpcplugin/main_test.go b/plugin/rpcplugin/main_test.go
index 6cdd46df0..06423106c 100644
--- a/plugin/rpcplugin/main_test.go
+++ b/plugin/rpcplugin/main_test.go
@@ -10,11 +10,21 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/mattermost/mattermost-server/mlog"
 	"github.com/mattermost/mattermost-server/plugin/plugintest"
 	"github.com/mattermost/mattermost-server/plugin/rpcplugin/rpcplugintest"
 )
 
 func TestMain(t *testing.T) {
+	// Setup a global logger to catch tests logging outside of app context
+	// The global logger will be stomped by apps initalizing but that's fine for testing. Ideally this won't happen.
+	mlog.InitGlobalLogger(mlog.NewLogger(&mlog.LoggerConfiguration{
+		EnableConsole: true,
+		ConsoleJson:   true,
+		ConsoleLevel:  "error",
+		EnableFile:    false,
+	}))
+
 	dir, err := ioutil.TempDir("", "")
 	require.NoError(t, err)
 	defer os.RemoveAll(dir)
@@ -46,7 +56,7 @@ func TestMain(t *testing.T) {
 
 	var api plugintest.API
 
-	hooks, err := ConnectMain(muxer)
+	hooks, err := ConnectMain(muxer, "plugin_id")
 	require.NoError(t, err)
 	assert.NoError(t, hooks.OnActivate(&api))
 	assert.NoError(t, hooks.OnDeactivate())
diff --git a/plugin/rpcplugin/rpcplugintest/supervisor.go b/plugin/rpcplugin/rpcplugintest/supervisor.go
index 05dc8ed8f..2ae065621 100644
--- a/plugin/rpcplugin/rpcplugintest/supervisor.go
+++ b/plugin/rpcplugin/rpcplugintest/supervisor.go
@@ -7,6 +7,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"testing"
@@ -30,6 +32,7 @@ func TestSupervisorProvider(t *testing.T, sp SupervisorProviderFunc) {
 		"Supervisor_NonExistentExecutablePath": testSupervisor_NonExistentExecutablePath,
 		"Supervisor_StartTimeout":              testSupervisor_StartTimeout,
 		"Supervisor_PluginCrash":               testSupervisor_PluginCrash,
+		"Supervisor_PluginRepeatedlyCrash":     testSupervisor_PluginRepeatedlyCrash,
 	} {
 		t.Run(name, func(t *testing.T) { f(t, sp) })
 	}
@@ -188,3 +191,83 @@ func testSupervisor_PluginCrash(t *testing.T, sp SupervisorProviderFunc) {
 	assert.True(t, recovered)
 	require.NoError(t, supervisor.Stop())
 }
+
+// Crashed plugins should be relaunched at most three times.
+func testSupervisor_PluginRepeatedlyCrash(t *testing.T, sp SupervisorProviderFunc) {
+	dir, err := ioutil.TempDir("", "")
+	require.NoError(t, err)
+	defer os.RemoveAll(dir)
+
+	backend := filepath.Join(dir, "backend.exe")
+	CompileGo(t, `
+		package main
+
+		import (
+			"net/http"
+			"os"
+
+			"github.com/mattermost/mattermost-server/plugin/rpcplugin"
+		)
+
+		type MyPlugin struct {
+			crashing bool
+		}
+
+		func (p *MyPlugin) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+			if r.Method == http.MethodPost {
+				p.crashing = true
+				go func() {
+					os.Exit(1)
+				}()
+			}
+
+			if p.crashing {
+				w.WriteHeader(http.StatusInternalServerError)
+			} else {
+				w.WriteHeader(http.StatusOK)
+			}
+		}
+
+		func main() {
+			rpcplugin.Main(&MyPlugin{})
+		}
+	`, backend)
+
+	ioutil.WriteFile(filepath.Join(dir, "plugin.json"), []byte(`{"id": "foo", "backend": {"executable": "backend.exe"}}`), 0600)
+
+	var api plugintest.API
+	bundle := model.BundleInfoForPath(dir)
+	supervisor, err := sp(bundle)
+	require.NoError(t, err)
+	require.NoError(t, supervisor.Start(&api))
+
+	for attempt := 1; attempt <= 4; attempt++ {
+		// Verify that the plugin is operational
+		response := httptest.NewRecorder()
+		supervisor.Hooks().ServeHTTP(response, httptest.NewRequest(http.MethodGet, "/plugins/id", nil))
+		require.Equal(t, http.StatusOK, response.Result().StatusCode)
+
+		// Crash the plugin
+		supervisor.Hooks().ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/plugins/id", nil))
+
+		// Wait for it to potentially recover
+		recovered := false
+		for i := 0; i < 125; i++ {
+			response := httptest.NewRecorder()
+			supervisor.Hooks().ServeHTTP(response, httptest.NewRequest(http.MethodGet, "/plugins/id", nil))
+			if response.Result().StatusCode == http.StatusOK {
+				recovered = true
+				break
+			}
+
+			time.Sleep(time.Millisecond * 100)
+		}
+
+		if attempt < 4 {
+			require.True(t, recovered, "failed to recover after attempt %d", attempt)
+		} else {
+			require.False(t, recovered, "unexpectedly recovered after attempt %d", attempt)
+		}
+	}
+	require.NoError(t, supervisor.Stop())
+}
diff --git a/plugin/rpcplugin/sandbox/main_test.go b/plugin/rpcplugin/sandbox/main_test.go
new file mode 100644
index 000000000..4be4a42af
--- /dev/null
+++ b/plugin/rpcplugin/sandbox/main_test.go
@@ -0,0 +1,18 @@
+package sandbox
+
+import (
+	"testing"
+
+	"github.com/mattermost/mattermost-server/mlog"
+)
+
+func TestMain(t *testing.T) {
+	// Setup a global logger to catch tests logging outside of app context
+	// The global logger will be stomped by apps initalizing but that's fine for testing. Ideally this won't happen.
+	mlog.InitGlobalLogger(mlog.NewLogger(&mlog.LoggerConfiguration{
+		EnableConsole: true,
+		ConsoleJson:   true,
+		ConsoleLevel:  "error",
+		EnableFile:    false,
+	}))
+}
diff --git a/plugin/rpcplugin/supervisor.go b/plugin/rpcplugin/supervisor.go
index 6a48cb5e8..6e26d5682 100644
--- a/plugin/rpcplugin/supervisor.go
+++ b/plugin/rpcplugin/supervisor.go
@@ -12,19 +12,26 @@ import (
 	"sync/atomic"
 	"time"
 
+	"github.com/mattermost/mattermost-server/mlog"
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/plugin"
 )
 
+const (
+	MaxProcessRestarts = 3
+)
+
 // Supervisor implements a plugin.Supervisor that launches the plugin in a separate process and
 // communicates via RPC.
 //
-// If the plugin unexpectedly exists, the supervisor will relaunch it after a short delay.
+// If the plugin unexpectedly exits, the supervisor will relaunch it after a short delay, but will
+// only restart a plugin at most three times.
 type Supervisor struct {
 	hooks      atomic.Value
 	done       chan bool
 	cancel     context.CancelFunc
 	newProcess func(context.Context) (Process, io.ReadWriteCloser, error)
+	pluginId   string
 }
 
 var _ plugin.Supervisor = (*Supervisor)(nil)
@@ -66,19 +73,28 @@ func (s *Supervisor) run(ctx context.Context, start chan<- error, api plugin.API
 		s.done <- true
 	}()
 	done := ctx.Done()
-	for {
+	for i := 0; i <= MaxProcessRestarts; i++ {
 		s.runPlugin(ctx, start, api)
 		select {
 		case <-done:
 			return
 		default:
 			start = nil
-			time.Sleep(time.Second)
+			if i < MaxProcessRestarts {
+				mlog.Debug("Plugin terminated unexpectedly", mlog.String("plugin_id", s.pluginId))
+				time.Sleep(time.Duration((1 + i*i)) * time.Second)
+			} else {
+				mlog.Debug("Plugin terminated unexpectedly too many times", mlog.String("plugin_id", s.pluginId), mlog.Int("max_process_restarts", MaxProcessRestarts))
+			}
 		}
 	}
 }
 
 func (s *Supervisor) runPlugin(ctx context.Context, start chan<- error, api plugin.API) error {
+	if start == nil {
+		mlog.Debug("Restarting plugin", mlog.String("plugin_id", s.pluginId))
+	}
+
 	p, ipc, err := s.newProcess(ctx)
 	if err != nil {
 		if start != nil {
@@ -100,7 +116,7 @@ func (s *Supervisor) runPlugin(ctx context.Context, start chan<- error, api plug
 		muxerClosed <- muxer.Close()
 	}()
 
-	hooks, err := ConnectMain(muxer)
+	hooks, err := ConnectMain(muxer, s.pluginId)
 	if err == nil {
 		err = hooks.OnActivate(api)
 	}
@@ -147,5 +163,5 @@ func SupervisorWithNewProcessFunc(bundle *model.BundleInfo, newProcess func(cont
 	if strings.HasPrefix(executable, "..") {
 		return nil, fmt.Errorf("invalid backend executable")
 	}
-	return &Supervisor{newProcess: newProcess}, nil
+	return &Supervisor{pluginId: bundle.Manifest.Id, newProcess: newProcess}, nil
 }
diff --git a/plugin/valid.go b/plugin/valid.go
new file mode 100644
index 000000000..62c594a1a
--- /dev/null
+++ b/plugin/valid.go
@@ -0,0 +1,32 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See License.txt for license information.
+
+package plugin
+
+import (
+	"regexp"
+	"unicode/utf8"
+)
+
+const (
+	MinIdLength = 3
+	MaxIdLength = 190
+)
+
+var ValidId *regexp.Regexp
+
+func init() {
+	ValidId = regexp.MustCompile(`^[a-zA-Z0-9-_\.]+$`)
+}
+
+func IsValidId(id string) bool {
+	if utf8.RuneCountInString(id) < MinIdLength {
+		return false
+	}
+
+	if utf8.RuneCountInString(id) > MaxIdLength {
+		return false
+	}
+
+	return ValidId.MatchString(id)
+}
diff --git a/plugin/valid_test.go b/plugin/valid_test.go
new file mode 100644
index 000000000..d47eeb58b
--- /dev/null
+++ b/plugin/valid_test.go
@@ -0,0 +1,32 @@
+package plugin_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/mattermost/mattermost-server/plugin"
+)
+
+func TestIsValid(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]bool{
+		"":    false,
+		"a":   false,
+		"ab":  false,
+		"abc": true,
+		"abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij":  true,
+		"abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij1": false,
+		"../path":                                 false,
+		"/etc/passwd":                             false,
+		"com.mattermost.plugin_with_features-0.9": true,
+		"PLUGINS-THAT-YELL-ARE-OK-2":              true,
+	}
+
+	for id, valid := range testCases {
+		t.Run(id, func(t *testing.T) {
+			assert.Equal(t, valid, plugin.IsValidId(id))
+		})
+	}
+}