From 2c9e71a2b38d4563a8750c19fa82d82c767489f1 Mon Sep 17 00:00:00 2001 From: kolaente Date: Mon, 27 Jul 2026 13:58:40 +0200 Subject: [PATCH] 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. --- pkg/routes/rate_limit.go | 8 +- pkg/routes/rate_limit_test.go | 120 +++++++++++++++++++++++++++ pkg/webtests/huma_rate_limit_test.go | 60 ++++++++++++++ 3 files changed, 186 insertions(+), 2 deletions(-) create mode 100644 pkg/routes/rate_limit_test.go create mode 100644 pkg/webtests/huma_rate_limit_test.go diff --git a/pkg/routes/rate_limit.go b/pkg/routes/rate_limit.go index 8ef22ef56..9e899109a 100644 --- a/pkg/routes/rate_limit.go +++ b/pkg/routes/rate_limit.go @@ -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) } diff --git a/pkg/routes/rate_limit_test.go b/pkg/routes/rate_limit_test.go new file mode 100644 index 000000000..b1f4b4bb6 --- /dev/null +++ b/pkg/routes/rate_limit_test.go @@ -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 . + +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) +} diff --git a/pkg/webtests/huma_rate_limit_test.go b/pkg/webtests/huma_rate_limit_test.go new file mode 100644 index 000000000..b08867510 --- /dev/null +++ b/pkg/webtests/huma_rate_limit_test.go @@ -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 . + +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()) + }) +}