From 470e4c9d66bbc6c5a85cd59b603f5c34f574e8a1 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Fri, 14 Apr 2023 13:32:05 +0530 Subject: [PATCH] MM-51786: Sentry crash while installing a blocked plugin (#22804) We would return "nil, nil" if a plugin was on the blocklist. This would cause a nil dereference panic while trying to add struct fields from manifest to the audit logs. To avoid it, we return an error explicitly and ignore the error ids to unnecessarily have it log an error. https://mattermost.atlassian.net/browse/MM-51786 ```release-note NONE ``` --- server/channels/app/plugin.go | 7 ++++++- server/channels/app/plugin_install.go | 11 +++++++---- server/channels/app/plugin_install_test.go | 19 +++++++++---------- server/i18n/en.json | 8 ++++++++ 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/server/channels/app/plugin.go b/server/channels/app/plugin.go index 210002a3ff..64e9d176f9 100644 --- a/server/channels/app/plugin.go +++ b/server/channels/app/plugin.go @@ -352,7 +352,7 @@ func (ch *Channels) syncPlugins() *model.AppError { } mlog.Info("Syncing plugin from file store", mlog.String("bundle", plugin.path)) - if _, err := ch.installPluginLocally(reader, signature, installPluginLocallyAlways); err != nil { + if _, err := ch.installPluginLocally(reader, signature, installPluginLocallyAlways); err != nil && err.Id != "app.plugin.blocked.app_error" && err.Id != "app.plugin.skip_installation.app_error" { mlog.Error("Failed to sync plugin from file store", mlog.String("bundle", plugin.path), mlog.Err(err)) } }(plugin) @@ -952,6 +952,11 @@ func (ch *Channels) processPrepackagedPlugins(pluginsDir string) []*plugin.Prepa defer wg.Done() p, err := ch.processPrepackagedPlugin(psPath) if err != nil { + var appErr *model.AppError + // A log line already appears if the plugin is on the blocklist + if errors.As(err, &appErr) && (appErr.Id == "app.plugin.blocked.app_error" || appErr.Id == "app.plugin.skip_installation.app_error") { + return + } mlog.Error("Failed to install prepackaged plugin", mlog.String("path", psPath.path), mlog.Err(err)) return } diff --git a/server/channels/app/plugin_install.go b/server/channels/app/plugin_install.go index 5db3e2cb77..2d666c7d47 100644 --- a/server/channels/app/plugin_install.go +++ b/server/channels/app/plugin_install.go @@ -92,7 +92,10 @@ func (ch *Channels) installPluginFromData(data model.PluginEventData) { manifest, appErr := ch.installPluginLocally(reader, signature, installPluginLocallyAlways) if appErr != nil { - mlog.Error("Failed to sync plugin from file store", mlog.String("bundle", plugin.path), mlog.Err(appErr)) + // A log line already appears if the plugin is on the blocklist or skipped + if appErr.Id != "app.plugin.blocked.app_error" && appErr.Id != "app.plugin.skip_installation.app_error" { + mlog.Error("Failed to sync plugin from file store", mlog.String("bundle", plugin.path), mlog.Err(appErr)) + } return } @@ -330,8 +333,8 @@ func (ch *Channels) installExtractedPlugin(manifest *model.Manifest, fromPluginD // Check plugin id is not blocked if plugin.PluginIDIsBlocked(manifest.Id) { - mlog.Debug("Skipping installation of plugin since plugin is on blocklist", mlog.String("plugin_id", manifest.Id)) - return nil, nil + mlog.Debug("Skipping installation of plugin since plugin is on blocklist. Some plugins are blocked because they are built into this version of Mattermost.", mlog.String("plugin_id", manifest.Id)) + return nil, model.NewAppError("installExtractedPlugin", "app.plugin.blocked.app_error", map[string]any{"Id": manifest.Id}, "", http.StatusInternalServerError) } // Check for plugins installed with the same ID. @@ -365,7 +368,7 @@ func (ch *Channels) installExtractedPlugin(manifest *model.Manifest, fromPluginD if version.LTE(existingVersion) { mlog.Debug("Skipping local installation of plugin since existing version is newer", mlog.String("plugin_id", manifest.Id)) - return nil, nil + return nil, model.NewAppError("installExtractedPlugin", "app.plugin.skip_installation.app_error", map[string]any{"Id": manifest.Id}, "", http.StatusInternalServerError) } } diff --git a/server/channels/app/plugin_install_test.go b/server/channels/app/plugin_install_test.go index 5fde414bd6..ecbb75e164 100644 --- a/server/channels/app/plugin_install_test.go +++ b/server/channels/app/plugin_install_test.go @@ -172,10 +172,9 @@ func TestInstallPluginLocally(t *testing.T) { defer th.TearDown() cleanExistingBundles(t, th) - manifest, appErr := installPlugin(t, th, "playbooks", "0.0.1", installPluginLocallyAlways) - require.Nil(t, appErr) - require.Nil(t, manifest) - + _, appErr := installPlugin(t, th, "playbooks", "0.0.1", installPluginLocallyAlways) + require.NotNil(t, appErr) + require.Equal(t, "app.plugin.blocked.app_error", appErr.Id) assertBundleInfoManifests(t, th, []*model.Manifest{}) }) @@ -222,9 +221,9 @@ func TestInstallPluginLocally(t *testing.T) { require.Nil(t, appErr) require.NotNil(t, existingManifest) - manifest, appErr := installPlugin(t, th, "valid", "0.0.1", installPluginLocallyOnlyIfNewOrUpgrade) - require.Nil(t, appErr) - require.Nil(t, manifest) + _, appErr = installPlugin(t, th, "valid", "0.0.1", installPluginLocallyOnlyIfNewOrUpgrade) + require.NotNil(t, appErr) + require.Equal(t, "app.plugin.skip_installation.app_error", appErr.Id) assertBundleInfoManifests(t, th, []*model.Manifest{existingManifest}) }) @@ -238,9 +237,9 @@ func TestInstallPluginLocally(t *testing.T) { require.Nil(t, appErr) require.NotNil(t, existingManifest) - manifest, appErr := installPlugin(t, th, "valid", "0.0.2", installPluginLocallyOnlyIfNewOrUpgrade) - require.Nil(t, appErr) - require.Nil(t, manifest) + _, appErr = installPlugin(t, th, "valid", "0.0.2", installPluginLocallyOnlyIfNewOrUpgrade) + require.NotNil(t, appErr) + require.Equal(t, "app.plugin.skip_installation.app_error", appErr.Id) assertBundleInfoManifests(t, th, []*model.Manifest{existingManifest}) }) diff --git a/server/i18n/en.json b/server/i18n/en.json index e91fbf2656..540a62bff5 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -5931,6 +5931,10 @@ "id": "app.oauth.update_app.updating.app_error", "translation": "We encountered an error updating the app." }, + { + "id": "app.plugin.blocked.app_error", + "translation": "Plugin {{.Id}} is on the block list. Some plugins are blocked because they are built into this version of Mattermost." + }, { "id": "app.plugin.cluster.save_config.app_error", "translation": "The plugin configuration in your config.json file must be updated manually when using ReadOnlyConfig with clustering enabled." @@ -6063,6 +6067,10 @@ "id": "app.plugin.signature_decode.app_error", "translation": "Unable to decode base64 signature." }, + { + "id": "app.plugin.skip_installation.app_error", + "translation": "Skipping installation of plugin {{.Id}} since existing version is equal or newer." + }, { "id": "app.plugin.store_bundle.app_error", "translation": "Unable to store the plugin to the configured file store."