handle RateLimiter initialization errors (#8199)

Previously, an error occuring in NewRateLimiter would return a nil
reference – which would be de-referenced just after, making the server
crash.

app/ratelimit.go

@@ -12,6 +12,7 @@ import (
 	l4g "github.com/alecthomas/log4go"
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/utils"
+	"github.com/pkg/errors"
 	throttled "gopkg.in/throttled/throttled.v2"
 	"gopkg.in/throttled/throttled.v2/store/memstore"
 )
@@ -23,11 +24,10 @@ type RateLimiter struct {
 	header               string
 }
 
-func NewRateLimiter(settings *model.RateLimitSettings) *RateLimiter {
+func NewRateLimiter(settings *model.RateLimitSettings) (*RateLimiter, error) {
 	store, err := memstore.New(*settings.MemoryStoreSize)
 	if err != nil {
-		l4g.Critical(utils.T("api.server.start_server.rate_limiting_memory_store"))
+		return nil, errors.Wrap(err, utils.T("api.server.start_server.rate_limiting_memory_store"))
-		return nil
 	}
 
 	quota := throttled.RateQuota{
@@ -37,8 +37,7 @@ func NewRateLimiter(settings *model.RateLimitSettings) *RateLimiter {
 
 	throttledRateLimiter, err := throttled.NewGCRARateLimiter(store, quota)
 	if err != nil {
-		l4g.Critical(utils.T("api.server.start_server.rate_limiting_rate_limiter"))
+		return nil, errors.Wrap(err, utils.T("api.server.start_server.rate_limiting_rate_limiter"))
-		return nil
 	}
 
 	return &RateLimiter{
@@ -46,7 +45,7 @@ func NewRateLimiter(settings *model.RateLimitSettings) *RateLimiter {
 		useAuth:              *settings.VaryByUser,
 		useIP:                *settings.VaryByRemoteAddr,
 		header:               settings.VaryByHeader,
-	}
+	}, nil
 }
 
 func (rl *RateLimiter) GenerateKey(r *http.Request) string {

app/ratelimit_test.go

@@ -25,6 +25,21 @@ func genRateLimitSettings(useAuth, useIP bool, header string) *model.RateLimitSe
 	}
 }
 
+func TestNewRateLimiterSuccess(t *testing.T) {
+	settings := genRateLimitSettings(false, false, "")
+	rateLimiter, err := NewRateLimiter(settings)
+	require.NotNil(t, rateLimiter)
+	require.NoError(t, err)
+}
+
+func TestNewRateLimiterFailure(t *testing.T) {
+	invalidSettings := genRateLimitSettings(false, false, "")
+	invalidSettings.MaxBurst = model.NewInt(-100)
+	rateLimiter, err := NewRateLimiter(invalidSettings)
+	require.Nil(t, rateLimiter)
+	require.Error(t, err)
+}
+
 func TestGenerateKey(t *testing.T) {
 	cases := []struct {
 		useAuth         bool
@@ -58,7 +73,7 @@ func TestGenerateKey(t *testing.T) {
 			req.Header.Set(tc.header, tc.headerResult)
 		}
 
-		rateLimiter := NewRateLimiter(genRateLimitSettings(tc.useAuth, tc.useIP, tc.header))
+		rateLimiter, _ := NewRateLimiter(genRateLimitSettings(tc.useAuth, tc.useIP, tc.header))
 
 		key := rateLimiter.GenerateKey(req)
 

app/server.go

@@ -124,9 +124,14 @@ func (a *App) StartServer() {
 	if *a.Config().RateLimitSettings.Enable {
 		l4g.Info(utils.T("api.server.start_server.rate.info"))
 
-		a.Srv.RateLimiter = NewRateLimiter(&a.Config().RateLimitSettings)
+		rateLimiter, err := NewRateLimiter(&a.Config().RateLimitSettings)
+		if err != nil {
+			l4g.Critical(err.Error())
+			return
+		}
 
-		handler = a.Srv.RateLimiter.RateLimitHandler(handler)
+		a.Srv.RateLimiter = rateLimiter
+		handler = rateLimiter.RateLimitHandler(handler)
 	}
 
 	a.Srv.Server = &http.Server{