fix(security): rate limit the websocket upgrade endpoint (#3348)

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{

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)

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 <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"
+)
+
+// 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"))
+		})
+	}
+}