fix: normalize path before rate-limit bucket keying (#27273)

Coder's rate limiter keyed its bucket on the raw, un-normalized request
path (`httprate.KeyByEndpoint` reads `r.URL.Path` directly). The
router's `singleSlashMW` already collapses redundant slashes so a
request like `/api/v2/users//validate-password` reaches the same handler
as the canonical path, but it never touched `r.URL.Path`, so the rate
limiter saw a different key and let a client bypass a limit it had
already hit just by respelling the URL.

`keyByNormalizedEndpoint` replaces `KeyByEndpoint` and runs `path.Clean`
on `r.URL.Path` before using it as the key, so equivalent paths share
one bucket. Includes a unit test at the key-function level and an
integration test (`TestRateLimitPathNormalization`) that reproduces the
bypass against a real server.

Fixes CDM-02-003 (Cure53). Refs
https://github.com/coder/security-disclosures/issues/166.

coderd/coderd_test.go

@@ -612,3 +612,51 @@ func TestRateLimitByUser(t *testing.T) {
 			"member should not be able to bypass rate limit")
 	})
 }
+
+// TestRateLimitPathNormalization is a regression test for CDM-02-003
+// (Cure53): a client could bypass a rate limit by inserting redundant
+// slashes into the request path. Coder's router still routes the
+// respelled path to the same handler as the canonical path, but the rate
+// limiter previously keyed its bucket on the raw, un-normalized path, so
+// the respelled request landed in a fresh bucket instead of the one
+// already exhausted by the canonical path.
+func TestRateLimitPathNormalization(t *testing.T) {
+	t.Parallel()
+
+	const rateLimit = 2
+
+	client := coderdtest.New(t, &coderdtest.Options{
+		LoginRateLimit: rateLimit,
+	})
+
+	ctx := testutil.Context(t, testutil.WaitLong)
+
+	post := func(path string) int {
+		req, err := http.NewRequestWithContext(ctx, http.MethodPost,
+			client.URL.String()+path, strings.NewReader(`{"password":"hunter2"}`))
+		require.NoError(t, err)
+		req.Header.Set("Content-Type", "application/json")
+
+		resp, err := client.HTTPClient.Do(req)
+		require.NoError(t, err)
+		defer resp.Body.Close()
+		return resp.StatusCode
+	}
+
+	// Exhaust the limit against the canonical path.
+	for i := range rateLimit {
+		require.Equal(t, http.StatusOK, post("/api/v2/users/validate-password"),
+			"request %d against the canonical path should succeed", i+1)
+	}
+
+	// The canonical path is now rate limited.
+	require.Equal(t, http.StatusTooManyRequests, post("/api/v2/users/validate-password"),
+		"canonical path should be rate limited after exhausting the limit")
+
+	// Respelling the same endpoint with redundant slashes must not grant a
+	// fresh bucket: it's the same handler, so it must still be limited.
+	require.Equal(t, http.StatusTooManyRequests, post("/api/v2/users//validate-password"),
+		"double-slash variant must share the canonical path's rate-limit bucket")
+	require.Equal(t, http.StatusTooManyRequests, post("/api/v2/users///validate-password"),
+		"triple-slash variant must share the canonical path's rate-limit bucket")
+}

coderd/httpmw/ratelimit.go

@@ -3,6 +3,7 @@ package httpmw
 import (
 	"fmt"
 	"net/http"
+	"path"
 	"strconv"
 	"sync/atomic"
 	"time"
@@ -85,7 +86,7 @@ func RateLimit(count int, window time.Duration) func(http.Handler) http.Handler
 				"%q provided but user is not %v",
 				codersdk.BypassRatelimitHeader, rbac.RoleOwner(),
 			)
-		}, httprate.KeyByEndpoint),
+		}, keyByNormalizedEndpoint),
 		httprate.WithLimitHandler(func(w http.ResponseWriter, r *http.Request) {
 			httpapi.Write(r.Context(), w, http.StatusTooManyRequests, codersdk.Response{
 				Message: fmt.Sprintf("You've been rate limited for sending more than %v requests in %v.", count, window),
@@ -94,6 +95,21 @@ func RateLimit(count int, window time.Duration) func(http.Handler) http.Handler
 	)
 }
 
+// keyByNormalizedEndpoint mirrors httprate.KeyByEndpoint, but cleans the
+// request path first. chi's router tolerates redundant slashes (see
+// singleSlashMW in coderd.go) and routes them to the same handler as the
+// canonical path, but only normalizes its internal route-matching path,
+// not r.URL.Path. Without normalizing here too, a client can respell a
+// path, for example inserting an extra slash, to get a fresh rate-limit
+// bucket for an endpoint it's already been throttled on.
+func keyByNormalizedEndpoint(r *http.Request) (string, error) {
+	p := r.URL.Path
+	if p == "" {
+		p = "/"
+	}
+	return path.Clean(p), nil
+}
+
 // RateLimitByAuthToken returns a handler that limits requests based on the
 // authentication token in the request.
 //

coderd/httpmw/ratelimit_test.go

@@ -49,6 +49,36 @@ func TestRateLimit(t *testing.T) {
 		}
 	})
 
+	t.Run("PathNormalizationBypass", func(t *testing.T) {
+		t.Parallel()
+		rtr := chi.NewRouter()
+		rtr.Use(httpmw.RateLimit(1, time.Second))
+		// A wildcard route so that requests for both the canonical path and
+		// its redundant-slash variants reach the same handler, mirroring
+		// how chi's router resolves /api/v2/users//validate-password to the
+		// same handler as /api/v2/users/validate-password in production.
+		rtr.Post("/*", func(rw http.ResponseWriter, r *http.Request) {
+			rw.WriteHeader(http.StatusOK)
+		})
+
+		remoteAddr := randRemoteAddr()
+		paths := []string{
+			"/api/v2/users/validate-password",
+			"/api/v2/users//validate-password",
+			"/api/v2/users///validate-password",
+			"/api/v2/users/validate-password",
+		}
+		for i, p := range paths {
+			req := httptest.NewRequest("POST", p, nil)
+			req.RemoteAddr = remoteAddr
+			rec := httptest.NewRecorder()
+			rtr.ServeHTTP(rec, req)
+			resp := rec.Result()
+			_ = resp.Body.Close()
+			require.Equal(t, i != 0, resp.StatusCode == http.StatusTooManyRequests, "request %d (%s)", i, p)
+		}
+	})
+
 	t.Run("RandomIPs", func(t *testing.T) {
 		t.Parallel()
 		rtr := chi.NewRouter()