From 0e00afd65f21afa7e67ab961d11f4c5753c0e68e Mon Sep 17 00:00:00 2001
From: Agniva De Sarker <agnivade@yahoo.co.in>
Date: Thu, 9 Dec 2021 14:17:41 +0530
Subject: [PATCH] MM-40469: Handle invalid response codes from plugins (#19136)

* MM-40469: Handle invalid response codes from plugins

This was an interesting crash detected via Sentry.

Typically, any HTTP status code outside 100-999 range
will cause a crash in the HTTP server. And `ExecuteCommand`
is the only plugin hook which returns a model.AppError
instead of error.

So an incorrect plugin implementation could return
a status code of 0, and crash the server. We handle that
by rewriting any illegal response code to 500.

https://mattermost.atlassian.net/browse/MM-40469

```release-note
NONE
```

* added warning

```release-note
NONE
```
---
 app/plugin_commands.go      |  8 +++++
 app/plugin_commands_test.go | 61 +++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/app/plugin_commands.go b/app/plugin_commands.go
index edc5a2cc2a..48c0dc8811 100644
--- a/app/plugin_commands.go
+++ b/app/plugin_commands.go
@@ -13,6 +13,7 @@ import (
 
 	"github.com/mattermost/mattermost-server/v6/app/request"
 	"github.com/mattermost/mattermost-server/v6/model"
+	"github.com/mattermost/mattermost-server/v6/shared/mlog"
 )
 
 type PluginCommand struct {
@@ -164,5 +165,12 @@ func (a *App) tryExecutePluginCommand(c *request.Context, args *model.CommandArg
 		errMessage := fmt.Sprintf("err= Plugin %s crashed due to /%s command", matched.PluginId, trigger)
 		return matched.Command, nil, model.NewAppError("ExecutePluginCommand", "model.plugin_command_crash.error.app_error", map[string]interface{}{"Command": trigger, "PluginId": matched.PluginId}, errMessage, http.StatusInternalServerError)
 	}
+	// This is a response from the plugin, which may set an incorrect status code;
+	// e.g setting a status code of 0 will crash the server. So we always bucket everything under 500.
+	if appErr != nil && (appErr.StatusCode < 100 || appErr.StatusCode > 999) {
+		mlog.Warn("Invalid status code returned from plugin. Converting to internal server error.", mlog.String("plugin_id", matched.PluginId), mlog.Int("status_code", appErr.StatusCode))
+		appErr.StatusCode = http.StatusInternalServerError
+	}
+
 	return matched.Command, response, appErr
 }
diff --git a/app/plugin_commands_test.go b/app/plugin_commands_test.go
index f611f85364..5c3ffc4a64 100644
--- a/app/plugin_commands_test.go
+++ b/app/plugin_commands_test.go
@@ -377,4 +377,65 @@ func TestPluginCommand(t *testing.T) {
 		th.App.ch.RemovePlugin(pluginIDs[0])
 	})
 
+	t.Run("plugin returning status code 0", func(t *testing.T) {
+		th.App.UpdateConfig(func(cfg *model.Config) {
+			cfg.PluginSettings.Plugins["testloadpluginconfig"] = map[string]interface{}{
+				"TeamId": args.TeamId,
+			}
+		})
+
+		tearDown, _, activationErrors := SetAppEnvironmentWithPlugins(t, []string{`
+			package main
+
+			import (
+				"github.com/mattermost/mattermost-server/v6/plugin"
+				"github.com/mattermost/mattermost-server/v6/model"
+			)
+
+			type configuration struct {
+				TeamId string
+			}
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+
+				configuration configuration
+			}
+
+			func (p *MyPlugin) OnActivate() error {
+				err := p.API.RegisterCommand(&model.Command{
+					TeamId: p.configuration.TeamId,
+					Trigger: "plugin",
+					DisplayName: "Plugin Command",
+					AutoComplete: true,
+					AutoCompleteDesc: "autocomplete",
+				})
+				if err != nil {
+					p.API.LogError("error", "err", err)
+				}
+
+				return err
+			}
+
+			func (p *MyPlugin) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*model.CommandResponse, *model.AppError) {
+				return nil, &model.AppError{
+					Message: "error",
+					StatusCode: 0,
+				}
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+		`}, th.App, th.NewPluginAPI)
+		defer tearDown()
+		require.Len(t, activationErrors, 1)
+		require.Nil(t, nil, activationErrors[0])
+
+		args.Command = "/plugin"
+		_, err := th.App.ExecuteCommand(th.Context, args)
+		require.NotNil(t, err)
+		require.Equal(t, 500, err.StatusCode)
+	})
+
 }