Hash key for plugin store table and limit plugin ID length (#7915)

* Hash plugin store keys and update column limits

* Limit plugin ID length on install

* Add note to manifest id and allow zero length keys

6 files changed, 35 insertions, 17 deletions

diff --git a/app/plugin.go b/app/plugin.go
index 29031b9d7..e8844d821 100644
--- a/app/plugin.go
+++ b/app/plugin.go
@@ -6,12 +6,15 @@ package app
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
+	"encoding/base64"
 	"io"
 	"io/ioutil"
 	"net/http"
 	"os"
 	"path/filepath"
 	"strings"
+	"unicode/utf8"
 
 	l4g "github.com/alecthomas/log4go"
 
@@ -27,6 +30,10 @@ import (
 	"github.com/mattermost/mattermost-server/plugin/pluginenv"
 )
 
+const (
+	PLUGIN_MAX_ID_LENGTH = 200
+)
+
 var prepackagedPlugins map[string]func(string) ([]byte, error) = map[string]func(string) ([]byte, error){
 	"jira": jira.Asset,
 }
@@ -147,6 +154,10 @@ 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)
+	}
+
 	bundles, err := a.PluginEnv.Plugins()
 	if err != nil {
 		return nil, model.NewAppError("installPlugin", "app.plugin.install.app_error", nil, err.Error(), http.StatusInternalServerError)
@@ -473,10 +484,16 @@ func (a *App) ShutDownPlugins() {
 	a.PluginEnv = nil
 }
 
+func getKeyHash(key string) string {
+	hash := sha256.New()
+	hash.Write([]byte(key))
+	return base64.StdEncoding.EncodeToString(hash.Sum(nil))
+}
+
 func (a *App) SetPluginKey(pluginId string, key string, value []byte) *model.AppError {
 	kv := &model.PluginKeyValue{
 		PluginId: pluginId,
-		Key:      key,
+		Key:      getKeyHash(key),
 		Value:    value,
 	}
 
@@ -490,7 +507,7 @@ func (a *App) SetPluginKey(pluginId string, key string, value []byte) *model.App
 }
 
 func (a *App) GetPluginKey(pluginId string, key string) ([]byte, *model.AppError) {
-	result := <-a.Srv.Store.Plugin().Get(pluginId, key)
+	result := <-a.Srv.Store.Plugin().Get(pluginId, getKeyHash(key))
 
 	if result.Err != nil {
 		if result.Err.StatusCode == http.StatusNotFound {
@@ -506,7 +523,7 @@ func (a *App) GetPluginKey(pluginId string, key string) ([]byte, *model.AppError
 }
 
 func (a *App) DeletePluginKey(pluginId string, key string) *model.AppError {
-	result := <-a.Srv.Store.Plugin().Delete(pluginId, key)
+	result := <-a.Srv.Store.Plugin().Delete(pluginId, getKeyHash(key))
 
 	if result.Err != nil {
 		l4g.Error(result.Err.Error())
diff --git a/i18n/en.json b/i18n/en.json
index 6ce9b19dc..2ff55d24f 100644
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -3559,18 +3559,14 @@
     "translation": "[{{ .SiteName }}] Notification in {{ .TeamName}} on {{.Month}} {{.Day}}, {{.Year}}"
   },
   {
-    "id": "app.plugin.prepackaged.app_error",
-    "translation": "Prepackaged plugins cannot be modified."
-  },
-  {
-    "id": "app.plugin.key_value.set.app_error",
-    "translation": "Unable to set key value. See detailed error for more information."
-  },
-  {
     "id": "app.plugin.activate.app_error",
     "translation": "Unable to activate extracted plugin. Plugin may already exist and be activated."
   },
   {
+    "id": "app.plugin.id_length.app_error",
+    "translation": "Plugin Id must be less than {{.Max}} characters."
+  },
+  {
     "id": "app.plugin.config.app_error",
     "translation": "Error saving plugin state in config"
   },
diff --git a/model/manifest.go b/model/manifest.go
index 121d3e0d2..7d28f6dc7 100644
--- a/model/manifest.go
+++ b/model/manifest.go
@@ -102,8 +102,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. 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 are limited
+	// to 200 characters. Reverse-DNS notation using a name you control is a good option.
+	// For example, "com.mycompany.myplugin".
 	Id string `json:"id" yaml:"id"`
 
 	// The name to be displayed for the plugin.
diff --git a/model/plugin_key_value.go b/model/plugin_key_value.go
index ceb216c2a..093422c82 100644
--- a/model/plugin_key_value.go
+++ b/model/plugin_key_value.go
@@ -9,8 +9,8 @@ import (
 )
 
 const (
-	KEY_VALUE_PLUGIN_ID_MAX_RUNES = 100
-	KEY_VALUE_KEY_MAX_RUNES       = 100
+	KEY_VALUE_PLUGIN_ID_MAX_RUNES = 200
+	KEY_VALUE_KEY_MAX_RUNES       = 50
 )
 
 type PluginKeyValue struct {
diff --git a/store/sqlstore/plugin_store.go b/store/sqlstore/plugin_store.go
index a4b49cb27..66a082cdf 100644
--- a/store/sqlstore/plugin_store.go
+++ b/store/sqlstore/plugin_store.go
@@ -21,8 +21,8 @@ func NewSqlPluginStore(sqlStore SqlStore) store.PluginStore {
 
 	for _, db := range sqlStore.GetAllConns() {
 		table := db.AddTableWithName(model.PluginKeyValue{}, "PluginKeyValueStore").SetKeys(false, "PluginId", "Key")
-		table.ColMap("PluginId").SetMaxSize(100)
-		table.ColMap("Key").SetMaxSize(100)
+		table.ColMap("PluginId").SetMaxSize(200)
+		table.ColMap("Key").SetMaxSize(50)
 		table.ColMap("Value").SetMaxSize(8192)
 	}
 
diff --git a/store/sqlstore/upgrade.go b/store/sqlstore/upgrade.go
index d10235135..55a408c7f 100644
--- a/store/sqlstore/upgrade.go
+++ b/store/sqlstore/upgrade.go
@@ -323,6 +323,10 @@ func UpgradeDatabaseToVersion44(sqlStore SqlStore) {
 }
 
 func UpgradeDatabaseToVersion45(sqlStore SqlStore) {
+	//TODO: Remove next two lines before 4.5 release. They're just here to fix CI servers
+	sqlStore.AlterColumnTypeIfExists("PluginKeyValueStore", "PluginId", "varchar(50)", "varchar(50)")
+	sqlStore.AlterColumnTypeIfExists("PluginKeyValueStore", "PKey", "varchar(200)", "varchar(200)")
+
 	//TODO: Uncomment when 4.5 is released
 	/*if shouldPerformUpgrade(sqlStore, VERSION_4_4_0, VERSION_4_5_0) {