fix(ratelimit): don't panic on unauthenticated requests

With ratelimit.kind at its default value of "user", the rate limit middleware logged the error from GetAuthFromClaims and then dereferenced the nil web.Auth anyway. Every unauthenticated /api/v2 request produces exactly that state, since v2 attaches the limiter to the single group serving its public routes too - so enabling rate limiting turned /api/v2/info, /api/v2/health and /api/v2/login into 500s. v1 is unaffected because it splits its unauthenticated routes into their own ip-keyed subgroups before the "user" limiter is attached. Fall back to keying by IP, matching the "ip" kind and v1's unauthenticated groups. Authenticated requests are unchanged.
2026-08-23 17:46:28 -05:00 · 2026-07-27 22:31:06 +00:00
parent 9617a9bee8
commit 2c9e71a2b3
3 changed files with 186 additions and 2 deletions
@@ -41,10 +41,14 @@ func RateLimit(rateLimiter *limiter.Limiter, rateLimitKind string) echo.Middlewa
 				rateLimitKey = c.RealIP()
 			case "user":
 				auth, err := auth2.GetAuthFromClaims(c)
-				if err != nil {
+				// Unauthenticated requests hit this middleware because v2 rate limits
+				// one group covering its public routes as well - key those by IP.
+				if err != nil || auth == nil {
 					log.Errorf("Error getting auth from jwt claims: %v", err)
+					rateLimitKey = "ip_" + c.RealIP()
+				} else {
+					rateLimitKey = "user_" + strconv.FormatInt(auth.GetID(), 10)
 				}
-				rateLimitKey = "user_" + strconv.FormatInt(auth.GetID(), 10)
 			default:
 				log.Errorf("Unknown rate limit kind configured: %s", rateLimitKind)
 			}
@@ -0,0 +1,120 @@
+// Vikunja is a to-do list application to facilitate your life.
+// Copyright 2018-present Vikunja and contributors. All rights reserved.
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+package routes
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"time"
+
+	"code.vikunja.io/api/pkg/log"
+	auth2 "code.vikunja.io/api/pkg/modules/auth"
+
+	"github.com/golang-jwt/jwt/v5"
+	"github.com/labstack/echo/v5"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/ulule/limiter/v3"
+	"github.com/ulule/limiter/v3/drivers/store/memory"
+)
+
+func newRateLimitTestContext(e *echo.Echo, remoteAddr string) (*echo.Context, *httptest.ResponseRecorder) {
+	req := httptest.NewRequest(http.MethodGet, "/api/v2/info", nil)
+	req.RemoteAddr = remoteAddr
+	rec := httptest.NewRecorder()
+	return e.NewContext(req, rec), rec
+}
+
+func rateLimitTestHandler(rateLimitKind string, limit int64) (handler echo.HandlerFunc, called *int) {
+	calls := 0
+	rateLimiter := limiter.New(memory.NewStore(), limiter.Rate{Period: time.Minute, Limit: limit})
+	h := RateLimit(rateLimiter, rateLimitKind)(func(_ *echo.Context) error {
+		calls++
+		return nil
+	})
+	return h, &calls
+}
+
+// TestRateLimitUnauthenticated makes sure an unauthenticated request does not
+// panic when the rate limit kind is "user" - the v2 group rate limits its public
+// routes as well, so there are no jwt claims to key on.
+func TestRateLimitUnauthenticated(t *testing.T) {
+	log.InitLogger()
+	e := echo.New()
+
+	t.Run("does not panic and passes through", func(t *testing.T) {
+		h, calls := rateLimitTestHandler("user", 100)
+		c, rec := newRateLimitTestContext(e, "1.2.3.4:1234")
+
+		require.NotPanics(t, func() {
+			require.NoError(t, h(c))
+		})
+		assert.Equal(t, 1, *calls)
+		assert.Equal(t, "100", rec.Header().Get("X-RateLimit-Limit"))
+		assert.Equal(t, "99", rec.Header().Get("X-RateLimit-Remaining"))
+	})
+
+	t.Run("limits by ip", func(t *testing.T) {
+		h, calls := rateLimitTestHandler("user", 2)
+
+		for range 2 {
+			c, _ := newRateLimitTestContext(e, "1.2.3.4:1234")
+			require.NoError(t, h(c))
+		}
+
+		c, _ := newRateLimitTestContext(e, "1.2.3.4:1234")
+		err := h(c)
+		var he *echo.HTTPError
+		require.ErrorAs(t, err, &he)
+		assert.Equal(t, http.StatusTooManyRequests, he.Code)
+
+		// A different ip must have its own bucket, otherwise everyone shares one limit
+		other, _ := newRateLimitTestContext(e, "5.6.7.8:1234")
+		require.NoError(t, h(other))
+		assert.Equal(t, 3, *calls)
+	})
+}
+
+// TestRateLimitUser makes sure authenticated requests are still keyed by user id.
+func TestRateLimitUser(t *testing.T) {
+	log.InitLogger()
+	e := echo.New()
+
+	token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
+		"type":     float64(auth2.AuthTypeUser),
+		"id":       float64(42),
+		"username": "user42",
+	})
+
+	h, calls := rateLimitTestHandler("user", 2)
+
+	for range 2 {
+		c, _ := newRateLimitTestContext(e, "1.2.3.4:1234")
+		c.Set("user", token)
+		require.NoError(t, h(c))
+	}
+
+	// Same user from a different ip shares the bucket
+	c, _ := newRateLimitTestContext(e, "5.6.7.8:1234")
+	c.Set("user", token)
+	err := h(c)
+	var he *echo.HTTPError
+	require.ErrorAs(t, err, &he)
+	assert.Equal(t, http.StatusTooManyRequests, he.Code)
+	assert.Equal(t, 2, *calls)
+}
@@ -0,0 +1,60 @@
+// Vikunja is a to-do list application to facilitate your life.
+// Copyright 2018-present Vikunja and contributors. All rights reserved.
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+package webtests
+
+import (
+	"net/http"
+	"testing"
+
+	"code.vikunja.io/api/pkg/config"
+	"code.vikunja.io/api/pkg/routes"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestHumaRateLimitUnauthenticated covers rate limiting with the default "user"
+// kind: v2 rate limits one group which also serves its public routes, so those
+// requests reach the middleware without any claims to key on.
+func TestHumaRateLimitUnauthenticated(t *testing.T) {
+	_, err := setupTestEnv()
+	require.NoError(t, err)
+
+	config.RateLimitEnabled.Set(true)
+	config.RateLimitKind.Set("user")
+	config.RateLimitLimit.Set(2)
+	defer func() {
+		config.RateLimitEnabled.Set(false)
+		config.RateLimitLimit.Set(100)
+	}()
+
+	e := routes.NewEcho()
+	routes.RegisterRoutes(e)
+
+	for _, path := range []string{"/api/v2/info", "/api/v2/health"} {
+		t.Run(path, func(t *testing.T) {
+			rec := humaRequest(t, e, http.MethodGet, path, "", "", "")
+			assert.NotEqual(t, http.StatusInternalServerError, rec.Code, "body: %s", rec.Body.String())
+			assert.Equal(t, "2", rec.Header().Get("X-RateLimit-Limit"))
+		})
+	}
+
+	t.Run("limits by ip", func(t *testing.T) {
+		rec := humaRequest(t, e, http.MethodGet, "/api/v2/info", "", "", "")
+		assert.Equal(t, http.StatusTooManyRequests, rec.Code, "body: %s", rec.Body.String())
+	})
+}