Prominent payload limit error and configurable URL length limit (#27747)

* Added context error handler for MaxBytesError

* Made URL length limit configurable

* Added tests

* Removed an unused function

* Typo

server/channels/web/handlers.go

@@ -6,6 +6,7 @@ package web
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"reflect"
@@ -31,8 +32,7 @@ import (
 )
 
 const (
-	frameAncestors   = "'self' teams.microsoft.com"
+	frameAncestors = "'self' teams.microsoft.com"
-	maxURLCharacters = 2048
 )
 
 func GetHandlerName(h func(*Context, http.ResponseWriter, *http.Request)) string {
@@ -144,20 +144,15 @@ func generateDevCSP(c Context) string {
 	return " " + strings.Join(devCSP, " ")
 }
 
-func (h Handler) basicSecurityChecks(w http.ResponseWriter, r *http.Request) *model.AppError {
+func (h Handler) basicSecurityChecks(c *Context, w http.ResponseWriter, r *http.Request) {
+	maxURLCharacters := *c.App.Config().ServiceSettings.MaximumURLLength
 	if len(r.RequestURI) > maxURLCharacters {
-		return model.NewAppError("basicSecurityChecks", "basic_security_check.url.too_long_error", nil, "", http.StatusRequestURITooLong)
+		c.Err = model.NewAppError("basicSecurityChecks", "basic_security_check.url.too_long_error", nil, "", http.StatusRequestURITooLong)
+		return
 	}
-
-	return nil
 }
 
 func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	if appErr := h.basicSecurityChecks(w, r); appErr != nil {
-		http.Error(w, appErr.Error(), appErr.StatusCode)
-		return
-	}
-
 	w = newWrappedWriter(w)
 	now := time.Now()
 
@@ -202,6 +197,12 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	c.Params = ParamsFromRequest(r)
 	c.Logger = c.App.Log()
 
+	h.basicSecurityChecks(c, w, r)
+	if c.Err != nil {
+		h.handleContextError(c, w, r)
+		return
+	}
+
 	if *c.App.Config().ServiceSettings.EnableOpenTracing {
 		span, ctx := tracing.StartRootSpanByContext(context.Background(), "web:ServeHTTP")
 		carrier := opentracing.HTTPHeadersCarrier(r.Header)
@@ -378,38 +379,8 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	// Handle errors that have occurred
 	if c.Err != nil {
-		c.Err.RequestId = c.AppContext.RequestId()
+		h.handleContextError(c, w, r)
-		c.LogErrorByCode(c.Err)
+		return
-		// The locale translation needs to happen after we have logged it.
-		// We don't want the server logs to be translated as per user locale.
-		c.Err.Translate(c.AppContext.T)
-
-		c.Err.Where = r.URL.Path
-
-		// Block out detailed error when not in developer mode
-		if !*c.App.Config().ServiceSettings.EnableDeveloper {
-			c.Err.WipeDetailed()
-		}
-
-		// Sanitize all 5xx error messages in hardened mode
-		if *c.App.Config().ServiceSettings.ExperimentalEnableHardenedMode && c.Err.StatusCode >= 500 {
-			c.Err.Id = ""
-			c.Err.Message = "Internal Server Error"
-			c.Err.WipeDetailed()
-			c.Err.StatusCode = 500
-			c.Err.Where = ""
-		}
-
-		if IsAPICall(c.App, r) || IsWebhookCall(c.App, r) || IsOAuthAPICall(c.App, r) || r.Header.Get("X-Mobile-App") != "" {
-			w.WriteHeader(c.Err.StatusCode)
-			w.Write([]byte(c.Err.ToJSON()))
-		} else {
-			utils.RenderWebAppError(c.App.Config(), w, r, c.Err, c.App.AsymmetricSigningKey())
-		}
-
-		if c.App.Metrics() != nil {
-			c.App.Metrics().IncrementHTTPError()
-		}
 	}
 
 	statusCode = strconv.Itoa(w.(*responseWriterWrapper).StatusCode())
@@ -429,6 +400,55 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
+func (h Handler) handleContextError(c *Context, w http.ResponseWriter, r *http.Request) {
+	if c.Err == nil {
+		return
+	}
+
+	// We're handling payload limit error here because it needs to be handled globally.
+	var maxBytesErr *http.MaxBytesError
+	// check if error is a MaxBytesError error, which occurs when you read more bytes from buffer than configured
+	if ok := errors.As(c.Err, &maxBytesErr); ok {
+		// replace the context error with this error if so,
+		newErr := model.NewAppError(c.Err.Where, "api.context.request_body_too_large.app_error", nil, "Use the setting `MaximumPayloadSizeBytes` in Mattermost config to configure allowed payload limit. Learn more about this setting in Mattermost docs at https://docs.mattermost.com/configure/environment-configuration-settings.html#maximum-payload-size", http.StatusRequestEntityTooLarge)
+		c.Err = newErr
+	}
+
+	c.Err.RequestId = c.AppContext.RequestId()
+	c.LogErrorByCode(c.Err)
+	// The locale translation needs to happen after we have logged it.
+	// We don't want the server logs to be translated as per user locale.
+	c.Err.Translate(c.AppContext.T)
+
+	c.Err.Where = r.URL.Path
+
+	// Block out detailed error when not in developer mode
+	if !*c.App.Config().ServiceSettings.EnableDeveloper {
+		c.Err.WipeDetailed()
+	}
+
+	// Sanitize all 5xx error messages in hardened mode
+	if *c.App.Config().ServiceSettings.ExperimentalEnableHardenedMode && c.Err.StatusCode >= 500 {
+		c.Err.Id = ""
+		c.Err.Message = "Internal Server Error"
+		c.Err.WipeDetailed()
+		c.Err.StatusCode = 500
+		c.Err.Where = ""
+	}
+
+	if IsAPICall(c.App, r) || IsWebhookCall(c.App, r) || IsOAuthAPICall(c.App, r) || r.Header.Get("X-Mobile-App") != "" {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(c.Err.StatusCode)
+		w.Write([]byte(c.Err.ToJSON()))
+	} else {
+		utils.RenderWebAppError(c.App.Config(), w, r, c.Err, c.App.AsymmetricSigningKey())
+	}
+
+	if c.App.Metrics() != nil {
+		c.App.Metrics().IncrementHTTPError()
+	}
+}
+
 type OriginClient string
 
 const (

server/channels/web/handlers_test.go

@@ -4,8 +4,11 @@
 package web
 
 import (
+	"encoding/json"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -927,3 +930,156 @@ func TestGetOriginClient(t *testing.T) {
 		require.Equal(t, tc.expectedClient, actualClient)
 	}
 }
+
+func noOpHandler(_ *Context, _ http.ResponseWriter, _ *http.Request) {
+	// no op
+}
+
+func TestHandlerServeHTTPBasicSecurityChecks(t *testing.T) {
+	t.Run("Should not cause 414 error if url is smaller than configured limit", func(t *testing.T) {
+		th := SetupWithStoreMock(t)
+		defer th.TearDown()
+
+		web := New(th.Server)
+		handler := web.NewHandler(noOpHandler)
+
+		// using the default URL length
+		request := httptest.NewRequest("GET", "/api/v4/test?with=not&many=query_params", nil)
+		response := httptest.NewRecorder()
+		handler.ServeHTTP(response, request)
+		assert.Equal(t, http.StatusOK, response.Code)
+	})
+
+	t.Run("Should cause 414 error if url is longer than configured limit", func(t *testing.T) {
+		th := SetupWithStoreMock(t)
+		defer th.TearDown()
+
+		mockStore := th.App.Srv().Store().(*mocks.Store)
+		mockUserStore := mocks.UserStore{}
+		mockUserStore.On("Count", mock.Anything).Return(int64(10), nil)
+		mockPostStore := mocks.PostStore{}
+		mockPostStore.On("GetMaxPostSize").Return(65535, nil)
+		mockSystemStore := mocks.SystemStore{}
+		mockSystemStore.On("GetByName", "UpgradedFromTE").Return(&model.System{Name: "UpgradedFromTE", Value: "false"}, nil)
+		mockSystemStore.On("GetByName", "InstallationDate").Return(&model.System{Name: "InstallationDate", Value: "10"}, nil)
+		mockSystemStore.On("GetByName", "FirstServerRunTimestamp").Return(&model.System{Name: "FirstServerRunTimestamp", Value: "10"}, nil)
+
+		mockStore.On("User").Return(&mockUserStore)
+		mockStore.On("Post").Return(&mockPostStore)
+		mockStore.On("System").Return(&mockSystemStore)
+		mockStore.On("GetDBSchemaVersion").Return(1, nil)
+
+		th.App.UpdateConfig(func(config *model.Config) {
+			config.ServiceSettings.MaximumURLLength = model.NewInt(10)
+		})
+
+		web := New(th.Server)
+		handler := web.NewHandler(noOpHandler)
+
+		request := httptest.NewRequest("GET", "/api/v4/test?a_url_longer_than_10_characters_including_query_params", nil)
+		response := httptest.NewRecorder()
+		handler.ServeHTTP(response, request)
+		assert.Equal(t, http.StatusRequestURITooLong, response.Code)
+	})
+
+	t.Run("414 error should include query params in computing URL length", func(t *testing.T) {
+		th := SetupWithStoreMock(t)
+		defer th.TearDown()
+
+		mockStore := th.App.Srv().Store().(*mocks.Store)
+		mockUserStore := mocks.UserStore{}
+		mockUserStore.On("Count", mock.Anything).Return(int64(10), nil)
+		mockPostStore := mocks.PostStore{}
+		mockPostStore.On("GetMaxPostSize").Return(65535, nil)
+		mockSystemStore := mocks.SystemStore{}
+		mockSystemStore.On("GetByName", "UpgradedFromTE").Return(&model.System{Name: "UpgradedFromTE", Value: "false"}, nil)
+		mockSystemStore.On("GetByName", "InstallationDate").Return(&model.System{Name: "InstallationDate", Value: "10"}, nil)
+		mockSystemStore.On("GetByName", "FirstServerRunTimestamp").Return(&model.System{Name: "FirstServerRunTimestamp", Value: "10"}, nil)
+
+		mockStore.On("User").Return(&mockUserStore)
+		mockStore.On("Post").Return(&mockPostStore)
+		mockStore.On("System").Return(&mockSystemStore)
+		mockStore.On("GetDBSchemaVersion").Return(1, nil)
+
+		th.App.UpdateConfig(func(config *model.Config) {
+			config.ServiceSettings.MaximumURLLength = model.NewInt(20)
+		})
+
+		web := New(th.Server)
+		handler := web.NewHandler(noOpHandler)
+
+		// this URL is within the 20 characters limit excluding query params.
+		// but this should still fail as URL length includes query params
+		request := httptest.NewRequest("GET", "/api/v4/test?a_url_longer_than_10_characters_including_query_params", nil)
+		response := httptest.NewRecorder()
+		handler.ServeHTTP(response, request)
+		assert.Equal(t, http.StatusRequestURITooLong, response.Code)
+	})
+}
+
+func TestHandlerServeHTTPRequestPayloadLimit(t *testing.T) {
+	jsonReaderHandler := func(context *Context, writer http.ResponseWriter, request *http.Request) {
+		// read request body into a string
+		var body *string
+		err := json.NewDecoder(request.Body).Decode(&body)
+		if err != nil {
+			fmt.Printf("Error occurred reading request body, error: %s", err.Error())
+			context.Err = model.NewAppError("TestHandlerServeHTTPRequestPayloadLimit", "", nil, "", http.StatusBadRequest).Wrap(err)
+		} else {
+			fmt.Printf("Received body- '%s'", *body)
+			writer.WriteHeader(http.StatusOK)
+		}
+	}
+
+	t.Run("should allow payload smaller than set limit", func(t *testing.T) {
+		th := SetupWithStoreMock(t)
+		defer th.TearDown()
+
+		web := New(th.Server)
+		handler := web.NewHandler(jsonReaderHandler)
+
+		body := strings.NewReader("\"a very small request body\"")
+		request := httptest.NewRequest("POST", "/api/v4/test", body)
+		response := httptest.NewRecorder()
+		handler.ServeHTTP(response, request)
+
+		assert.Equal(t, http.StatusOK, response.Code)
+	})
+
+	t.Run("Should error out when request body is larger than set limit", func(t *testing.T) {
+		th := SetupWithStoreMock(t)
+		defer th.TearDown()
+
+		mockStore := th.App.Srv().Store().(*mocks.Store)
+		mockUserStore := mocks.UserStore{}
+		mockUserStore.On("Count", mock.Anything).Return(int64(10), nil)
+		mockPostStore := mocks.PostStore{}
+		mockPostStore.On("GetMaxPostSize").Return(65535, nil)
+		mockSystemStore := mocks.SystemStore{}
+		mockSystemStore.On("GetByName", "UpgradedFromTE").Return(&model.System{Name: "UpgradedFromTE", Value: "false"}, nil)
+		mockSystemStore.On("GetByName", "InstallationDate").Return(&model.System{Name: "InstallationDate", Value: "10"}, nil)
+		mockSystemStore.On("GetByName", "FirstServerRunTimestamp").Return(&model.System{Name: "FirstServerRunTimestamp", Value: "10"}, nil)
+
+		mockStore.On("User").Return(&mockUserStore)
+		mockStore.On("Post").Return(&mockPostStore)
+		mockStore.On("System").Return(&mockSystemStore)
+		mockStore.On("GetDBSchemaVersion").Return(1, nil)
+
+		th.App.UpdateConfig(func(config *model.Config) {
+			config.ServiceSettings.MaximumPayloadSizeBytes = model.NewInt64(1)
+		})
+
+		web := New(th.Server)
+		handler := web.NewHandler(jsonReaderHandler)
+
+		// this is a 600 character long string.
+		// Even though we have set the max payload size to be 1, we still need at least 513 bytes (1 byte configured + bytes.MinRead = 1 + 512 = 513) bytes.
+		// This is because the buffer will always be at least bytes.MinRead bytes large, so the effective payload limit is bytes.MinRead + the configured value.
+		body := strings.NewReader("\"weunfrwghyajuaqqkrecexurpmrmgpimjieymiwfhfrrrgiqpqrznkjtubwcuybyixyxwtwddpytukritccyugyeuvdtzjkkyiwhquzqkrqkhgyyfqnquzchjqkrkzfrxthduzizqtdxzqirxhzihbivmkdwpbeddepdanzuuzqxbdqfvgkwumervhghywexitbjdnvxcniuamwmqdecbbqbgnjjqwkdcucvnpwynuruztpdtmmvbpkevurpjdwdhpayaindzmnkmyybudfkjdkqwuiviriudtqytybuwfkkwpepwhpekfewnxgpkfctdqjmemngvntnizvfznaiqpbumgtcxidvawtgcdyqijbxzrgezvjmcwikiabbpqabrwfgncrmvqththepffatnhchhnmrhkuqvgrzfugzhuwicaemhcacrazmgzmrgrkuhwucfydhwxfhzfukzjhvdxkuhzjrxwippxadvwzigndxwdxvganxggjjxwdqtgqgnpqqygndviadvttwfntcreitijaqrpfygdehbcftyfcjvrfwvjmbtdptutjgtbyhbyddfecyyujgrxyujzmryymj\"")
+		request := httptest.NewRequest("POST", "/api/v4/test", body)
+		response := httptest.NewRecorder()
+		handler.ServeHTTP(response, request)
+
+		assert.Equal(t, http.StatusRequestEntityTooLarge, response.Code)
+	})
+}

server/i18n/en.json

@@ -1785,6 +1785,10 @@
     "id": "api.context.remote_id_missing.app_error",
     "translation": "Secure connection ID missing."
   },
+  {
+    "id": "api.context.request_body_too_large.app_error",
+    "translation": "Unable to process request. Request body too large."
+  },
   {
     "id": "api.context.server_busy.app_error",
     "translation": "Server is busy, non-critical services are temporarily unavailable."
@@ -8762,6 +8766,10 @@
     "id": "model.config.is_valid.max_payload_size.app_error",
     "translation": "Invalid max payload size for service settings. Must be a whole number greater than zero."
   },
+  {
+    "id": "model.config.is_valid.max_url_length.app_error",
+    "translation": "Invalid max URL length for service settings. Must be a whole number greater than zero."
+  },
   {
     "id": "model.config.is_valid.max_users.app_error",
     "translation": "Invalid maximum users per team for team settings. Must be a positive number."

server/platform/services/telemetry/telemetry.go

@@ -493,6 +493,7 @@ func (ts *TelemetryService) trackConfig() {
 		"allow_synced_drafts":                                     *cfg.ServiceSettings.AllowSyncedDrafts,
 		"refresh_post_stats_run_time":                             *cfg.ServiceSettings.RefreshPostStatsRunTime,
 		"maximum_payload_size":                                    *cfg.ServiceSettings.MaximumPayloadSizeBytes,
+		"maximum_url_length":                                      *cfg.ServiceSettings.MaximumURLLength,
 	})
 
 	ts.SendTelemetry(TrackConfigTeam, map[string]any{

server/public/model/config.go

@@ -112,6 +112,7 @@ const (
 	ServiceSettingsDefaultGiphySdkKeyTest        = "s0glxvzVg9azvPipKxcPLpXV0q1x1fVP"
 	ServiceSettingsDefaultDeveloperFlags         = ""
 	ServiceSettingsDefaultUniqueReactionsPerPost = 50
+	ServiceSettingsDefaultMaxURLLength           = 2048
 	ServiceSettingsMaxUniqueReactionsPerPost     = 500
 
 	TeamSettingsDefaultSiteName              = "Mattermost"
@@ -410,6 +411,7 @@ type ServiceSettings struct {
 	UniqueEmojiReactionLimitPerPost                   *int    `access:"site_posts"`
 	RefreshPostStatsRunTime                           *string `access:"site_users_and_teams"`
 	MaximumPayloadSizeBytes                           *int64  `access:"environment_file_storage,write_restrictable,cloud_restrictable"`
+	MaximumURLLength                                  *int    `access:"environment_file_storage,write_restrictable,cloud_restrictable"`
 }
 
 var MattermostGiphySdkKey string
@@ -921,6 +923,10 @@ func (s *ServiceSettings) SetDefaults(isUpdate bool) {
 	if s.MaximumPayloadSizeBytes == nil {
 		s.MaximumPayloadSizeBytes = NewInt64(300000)
 	}
+
+	if s.MaximumURLLength == nil {
+		s.MaximumURLLength = NewInt(ServiceSettingsDefaultMaxURLLength)
+	}
 }
 
 type ClusterSettings struct {
@@ -4031,6 +4037,10 @@ func (s *ServiceSettings) isValid() *AppError {
 		return NewAppError("Config.IsValid", "model.config.is_valid.max_payload_size.app_error", nil, "", http.StatusBadRequest)
 	}
 
+	if *s.MaximumURLLength <= 0 {
+		return NewAppError("Config.IsValid", "model.config.is_valid.max_url_length.app_error", nil, "", http.StatusBadRequest)
+	}
+
 	if *s.ReadTimeout <= 0 {
 		return NewAppError("Config.IsValid", "model.config.is_valid.read_timeout.app_error", nil, "", http.StatusBadRequest)
 	}