From 79480494d02a5318dcef0bf27573f8fd5da0edc9 Mon Sep 17 00:00:00 2001 From: Harshil Sharma <18575143+harshilsharma63@users.noreply.github.com> Date: Thu, 25 Jul 2024 14:39:58 +0530 Subject: [PATCH] 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 | 106 +++++++----- server/channels/web/handlers_test.go | 156 ++++++++++++++++++ server/i18n/en.json | 8 + .../platform/services/telemetry/telemetry.go | 1 + server/public/model/config.go | 10 ++ 5 files changed, 238 insertions(+), 43 deletions(-) diff --git a/server/channels/web/handlers.go b/server/channels/web/handlers.go index 4d5d1e85c9..3d973fd5d6 100644 --- a/server/channels/web/handlers.go +++ b/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" - maxURLCharacters = 2048 + frameAncestors = "'self' teams.microsoft.com" ) 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() - 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.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() - } + h.handleContextError(c, w, r) + return } 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 ( diff --git a/server/channels/web/handlers_test.go b/server/channels/web/handlers_test.go index 48ce17c2c1..38fc5a900d 100644 --- a/server/channels/web/handlers_test.go +++ b/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) + }) +} diff --git a/server/i18n/en.json b/server/i18n/en.json index 774d93156c..7d88590fd1 100644 --- a/server/i18n/en.json +++ b/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." diff --git a/server/platform/services/telemetry/telemetry.go b/server/platform/services/telemetry/telemetry.go index d349322b17..972b4899a0 100644 --- a/server/platform/services/telemetry/telemetry.go +++ b/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{ diff --git a/server/public/model/config.go b/server/public/model/config.go index 876b8a38d7..a0a6413a12 100644 --- a/server/public/model/config.go +++ b/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) }