From ccc46508b0c58aef83eaa35bf5339c8da9c820fb Mon Sep 17 00:00:00 2001 From: Tink Date: Tue, 28 Jul 2026 17:12:18 +0200 Subject: [PATCH] fix(security): rate limit the websocket upgrade endpoint (#3348) --- pkg/routes/rate_limit.go | 10 +++++ pkg/routes/routes.go | 23 +++++------ pkg/webtests/ws_rate_limit_test.go | 63 ++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 13 deletions(-) create mode 100644 pkg/webtests/ws_rate_limit_test.go diff --git a/pkg/routes/rate_limit.go b/pkg/routes/rate_limit.go index b72d3559d..65ced2047 100644 --- a/pkg/routes/rate_limit.go +++ b/pkg/routes/rate_limit.go @@ -95,6 +95,16 @@ func createRateLimiter(rate limiter.Rate) *limiter.Limiter { return limiter.New(store, rate) } +// unauthRateLimit ignores RateLimitEnabled on purpose: pre-auth routes need a +// floor even with the global limiter off, which is the default. +func unauthRateLimit() echo.MiddlewareFunc { + rate := limiter.Rate{ + Period: 60 * time.Second, + Limit: config.RateLimitNoAuthRoutesLimit.GetInt64(), + } + return RateLimit(createRateLimiter(rate), "ip") +} + func setupRateLimit(a *echo.Group, rateLimitKind string) { if config.RateLimitEnabled.GetBool() { rate := limiter.Rate{ diff --git a/pkg/routes/routes.go b/pkg/routes/routes.go index 7d66d5df8..d29782fb4 100644 --- a/pkg/routes/routes.go +++ b/pkg/routes/routes.go @@ -91,7 +91,6 @@ import ( "github.com/getsentry/sentry-go" "github.com/labstack/echo/v5" "github.com/labstack/echo/v5/middleware" - "github.com/ulule/limiter/v3" ) // matchCORSOrigin checks if an origin matches any of the allowed origin patterns. @@ -318,13 +317,16 @@ func RegisterRoutes(e *echo.Echo) { })) } + // Shared across both API versions so the budget is per IP, not per version. + wsRateLimit := unauthRateLimit() + // API Routes a := e.Group("/api/v1") - registerAPIRoutes(a) + registerAPIRoutes(a, wsRateLimit) // /api/v2 — Huma-backed API, scaffolded alongside /api/v1. a2 := e.Group("/api/v2") - registerAPIRoutesV2(e, a2) + registerAPIRoutesV2(e, a2, wsRateLimit) // Collect routes for API token permissions // In Echo v5, we collect routes after registration using e.Router().Routes() @@ -438,7 +440,7 @@ func gateV2AdminRoutes() echo.MiddlewareFunc { // registerAPIRoutesV2 wires the /api/v2 Echo group. Token middleware is // attached before any route so Huma's spec and Scalar docs share the // resource handlers' stack; unauthenticatedAPIPaths keeps them public. -func registerAPIRoutesV2(e *echo.Echo, a *echo.Group) { +func registerAPIRoutesV2(e *echo.Echo, a *echo.Group, wsRateLimit echo.MiddlewareFunc) { a.Use(noStoreCacheControl()) a.Use(SetupTokenMiddleware()) // Match the authenticated v1 group: rate limiting and route metrics @@ -460,13 +462,13 @@ func registerAPIRoutesV2(e *echo.Echo, a *echo.Group) { // authenticates via its first message, so unauthenticatedAPIPaths exempts it // from the group's JWT middleware. Health and the Atom feed are Huma ops and // self-register via init()/RegisterAll. - a.GET("/ws", ws.UpgradeHandler) + a.GET("/ws", ws.UpgradeHandler, wsRateLimit) // Resources self-register via init(); RegisterAll runs them all + AutoPatch. apiv2.RegisterAll(api) } -func registerAPIRoutes(a *echo.Group) { +func registerAPIRoutes(a *echo.Group, wsRateLimit echo.MiddlewareFunc) { // Prevent browsers from caching API responses. Without an explicit // Cache-Control header browsers may heuristically cache JSON responses @@ -485,19 +487,14 @@ func registerAPIRoutes(a *echo.Group) { n.GET("/docs/redoc.standalone.js", apiv1.RedocJS) // WebSocket (auth happens after upgrade via first message) - n.GET("/ws", ws.UpgradeHandler) + n.GET("/ws", ws.UpgradeHandler, wsRateLimit) // Prometheus endpoint setupMetrics(n) // Separate route for unauthenticated routes to enable rate limits for it ur := a.Group("") - rate := limiter.Rate{ - Period: 60 * time.Second, - Limit: config.RateLimitNoAuthRoutesLimit.GetInt64(), - } - rateLimiter := createRateLimiter(rate) - ur.Use(RateLimit(rateLimiter, "ip")) + ur.Use(unauthRateLimit()) if config.AuthLocalEnabled.GetBool() { ur.POST("/register", apiv1.RegisterUser) diff --git a/pkg/webtests/ws_rate_limit_test.go b/pkg/webtests/ws_rate_limit_test.go new file mode 100644 index 000000000..37d3a4321 --- /dev/null +++ b/pkg/webtests/ws_rate_limit_test.go @@ -0,0 +1,63 @@ +// 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" +) + +// TestWebsocketUpgradeRateLimit asserts the /ws upgrade endpoint keeps its +// per-IP floor with the global rate limiter disabled - its default. The upgrade +// is unauthenticated, so without the floor anyone could spawn connections +// (2 goroutines + a socket each) at will. +func TestWebsocketUpgradeRateLimit(t *testing.T) { + _, err := setupTestEnv() + require.NoError(t, err) + require.False(t, config.RateLimitEnabled.GetBool(), "the global rate limiter must default to off for this test to be meaningful") + + previousLimit := config.RateLimitNoAuthRoutesLimit.GetInt64() + config.RateLimitNoAuthRoutesLimit.Set(2) + defer config.RateLimitNoAuthRoutesLimit.Set(previousLimit) + + e := routes.NewEcho() + routes.RegisterRoutes(e) + + // Both versions share one limiter, so the budget runs down across them + // instead of granting a fresh one per API version. + for _, path := range []string{"/api/v1/ws", "/api/v2/ws"} { + t.Run("within limit "+path, func(t *testing.T) { + rec := humaRequest(t, e, http.MethodGet, path, "", "", "") + assert.NotEqual(t, http.StatusTooManyRequests, rec.Code) + assert.Equal(t, "2", rec.Header().Get("X-RateLimit-Limit")) + }) + } + + for _, path := range []string{"/api/v1/ws", "/api/v2/ws"} { + t.Run("throttled "+path, func(t *testing.T) { + rec := humaRequest(t, e, http.MethodGet, path, "", "", "") + assert.Equal(t, http.StatusTooManyRequests, rec.Code, "body: %s", rec.Body.String()) + assert.Equal(t, "0", rec.Header().Get("X-RateLimit-Remaining")) + }) + } +}