fix #280: key rate-limit buckets on peer address, not client XFF

The deployed ingress does not append the peer to X-Forwarded-For, so the rightmost XFF entry was fully client-controlled and rotating the header gave a fresh rate-limit bucket per request (pentest: 8 creates, 6x201). clientIP() now defaults to the peer (RemoteAddr) and never trusts client-supplied headers. Per-client granularity is restored opt-in via PALETTE_CLIENT_IP_HEADER, honored only when the immediate peer is inside PALETTE_TRUSTED_PROXIES (default loopback + RFC1918) and the value parses as an IP.
2026-09-17 20:34:59 -05:00
parent 70b06db192
commit a0b34378a4
2 changed files with 166 additions and 75 deletions
@@ -1,85 +1,135 @@
 package api

-// Issue #85: the rate limit key must use the rightmost X-Forwarded-For entry
-// (appended by the trusted Traefik proxy), never the raw/leftmost header
-// value a client can forge. A spoofed FIRST XFF entry must not bypass the
-// limit or rotate buckets.
+// Issues #85 + #280: the rate limit bucket key must NEVER be derived from a
+// client-controlled header. The deployed ingress does not append the peer
+// address to X-Forwarded-For, so every XFF entry is attacker-controlled and
+// an attacker can rotate the header per request to get a fresh bucket. The
+// key defaults to the peer (RemoteAddr); an opt-in trusted header is honored
+// only when the immediate peer is a trusted proxy.

 import (
 	"bytes"
 	"net/http/httptest"
+	"sync"
 	"testing"
 )

-func TestClientIPTakesRightmostXFF(t *testing.T) {
-	r := httptest.NewRequest("POST", "/", nil)
-	r.RemoteAddr = "10.42.0.7:51000" // trusted Traefik pod
-	r.Header.Set("X-Forwarded-For", "1.2.3.4, 1.2.3.5, 203.0.113.9")
-	if got := clientIP(r); got != "203.0.113.9" {
-		t.Fatalf("clientIP = %q, want rightmost 203.0.113.9", got)
-	}
+// resetTrustedProxyCache clears the sync.Once-cached CIDR list so each test
+// re-parses PALETTE_TRUSTED_PROXIES under its own env.
+func resetTrustedProxyCache(t *testing.T) {
+	t.Helper()
+	t.Cleanup(func() {
+		trustedOnce = sync.Once{}
+		trustedCIDRs = nil
+	})
+	trustedOnce = sync.Once{}
+	trustedCIDRs = nil
 }

-func TestClientIPXRealIPFallback(t *testing.T) {
-	r := httptest.NewRequest("POST", "/", nil)
-	r.RemoteAddr = "10.42.0.7:51000"
-	r.Header.Set("X-Real-Ip", "203.0.113.10")
-	if got := clientIP(r); got != "203.0.113.10" {
-		t.Fatalf("clientIP = %q, want 203.0.113.10", got)
-	}
-}
-
-func TestClientIPDirectFallback(t *testing.T) {
-	r := httptest.NewRequest("POST", "/", nil)
-	r.RemoteAddr = "198.51.100.5:51000"
-	if got := clientIP(r); got != "198.51.100.5" {
-		t.Fatalf("clientIP = %q, want 198.51.100.5", got)
-	}
-}
-
-// TestRateLimitSpoofedFirstXFFDoesNotBypass: an attacker rotating a fake
-// leftmost XFF entry stays limited on their real (rightmost) IP.
-func TestRateLimitSpoofedFirstXFFDoesNotBypass(t *testing.T) {
+// Rotating XFF must not create fresh buckets: all requests share the peer's
+// bucket. This is the #280 pentest repro (8 creates, 6x201 before the fix).
+func TestRateLimitRotatingXFFSharesPeerBucket(t *testing.T) {
 	srv := newTestServer(t)
 	h := srv.routes()
-	for i := 0; i < 5; i++ {
+	codes := []int{}
+	for i := 0; i < 8; i++ {
 		req := httptest.NewRequest("POST", "/api/pastes", bytes.NewReader([]byte(`{"content":"hi"}`)))
-		req.RemoteAddr = "10.42.0.7:51000"
-		// each request spoofs a DIFFERENT leftmost entry
-		req.Header.Set("X-Forwarded-For", spoofN(i)+", 203.0.113.9")
+		req.RemoteAddr = "203.0.113.9:51000" // public peer
+		req.Header.Set("X-Forwarded-For", "9.9.9."+string(rune('0'+i)))
 		rr := httptest.NewRecorder()
 		h.ServeHTTP(rr, req)
-		if rr.Code != 201 {
-			t.Fatalf("req %d: want 201, got %d", i, rr.Code)
+		codes = append(codes, rr.Code)
+	}
+	allowed := 0
+	for _, c := range codes {
+		if c == 201 {
+			allowed++
 		}
 	}
-	// 6th request, still the same real IP, new spoofed prefix: must 429
+	if allowed > 5 {
+		t.Fatalf("rotating XFF bypassed the limit: allowed=%d codes=%v", allowed, codes)
+	}
+	// and the no-header bucket is the SAME bucket: fully exhausted
 	req := httptest.NewRequest("POST", "/api/pastes", bytes.NewReader([]byte(`{"content":"hi"}`)))
-	req.RemoteAddr = "10.42.0.7:51000"
-	req.Header.Set("X-Forwarded-For", "9.9.9.9, 203.0.113.9")
+	req.RemoteAddr = "203.0.113.9:51000"
 	rr := httptest.NewRecorder()
 	h.ServeHTTP(rr, req)
 	if rr.Code != 429 {
-		t.Fatalf("spoofed 6th req: want 429, got %d", rr.Code)
+		t.Fatalf("no-header request after rotating XFF: want 429, got %d", rr.Code)
 	}
 }

-func spoofN(i int) string {
-	return "1.2.3." + string(rune('0'+i))
+// clientIP unit checks.
+func TestClientIPIgnoresClientHeadersByDefault(t *testing.T) {
+	r := httptest.NewRequest("POST", "/", nil)
+	r.RemoteAddr = "198.51.100.5:51000"
+	r.Header.Set("X-Forwarded-For", "1.2.3.4, 5.6.7.8")
+	r.Header.Set("X-Real-Ip", "9.9.9.9")
+	if got := clientIP(r); got != "198.51.100.5" {
+		t.Fatalf("clientIP = %q, want peer 198.51.100.5", got)
+	}
 }

-// Distinct real IPs must still get distinct buckets (no over-limiting).
-func TestRateLimitDistinctRightmostIPsIndependent(t *testing.T) {
-	srv := newTestServer(t)
-	h := srv.routes()
-	for _, ip := range []string{"203.0.113.20", "203.0.113.21"} {
-		req := httptest.NewRequest("POST", "/api/pastes", bytes.NewReader([]byte(`{"content":"hi"}`)))
-		req.RemoteAddr = "10.42.0.7:51000"
-		req.Header.Set("X-Forwarded-For", "6.6.6.6, "+ip)
-		rr := httptest.NewRecorder()
-		h.ServeHTTP(rr, req)
-		if rr.Code != 201 {
-			t.Fatalf("ip %s: want 201, got %d", ip, rr.Code)
+// Opt-in trusted header: honored only from a trusted private peer and only
+// with a parseable IP value.
+func TestClientIPTrustedHeaderFromPrivatePeer(t *testing.T) {
+	t.Setenv("PALETTE_CLIENT_IP_HEADER", "Cf-Connecting-Ip")
+	// reset the cached CIDR parse (sync.Once) for a clean run
+	resetTrustedProxyCache(t)
+
+	r := httptest.NewRequest("POST", "/", nil)
+	r.RemoteAddr = "10.42.0.7:51000" // in-cluster proxy hop
+	r.Header.Set("Cf-Connecting-Ip", "203.0.113.42")
+	if got := clientIP(r); got != "203.0.113.42" {
+		t.Fatalf("clientIP = %q, want 203.0.113.42", got)
+	}
+	// same header from a PUBLIC peer must be ignored (peer not trusted)
+	r2 := httptest.NewRequest("POST", "/", nil)
+	r2.RemoteAddr = "203.0.113.9:51000"
+	r2.Header.Set("Cf-Connecting-Ip", "6.6.6.6")
+	if got := clientIP(r2); got != "203.0.113.9" {
+		t.Fatalf("clientIP = %q, want peer 203.0.113.9 (untrusted peer)", got)
+	}
+	// garbage header value must be ignored
+	r3 := httptest.NewRequest("POST", "/", nil)
+	r3.RemoteAddr = "10.42.0.7:51000"
+	r3.Header.Set("Cf-Connecting-Ip", "not-an-ip; 6.6.6.6")
+	if got := clientIP(r3); got != "10.42.0.7" {
+		t.Fatalf("clientIP = %q, want peer 10.42.0.7 (bad header value)", got)
+	}
+}
+
+// Custom trusted proxy CIDR list is honored.
+func TestClientIPCustomTrustedProxies(t *testing.T) {
+	t.Setenv("PALETTE_CLIENT_IP_HEADER", "Cf-Connecting-Ip")
+	t.Setenv("PALETTE_TRUSTED_PROXIES", "192.168.50.0/24")
+	resetTrustedProxyCache(t)
+
+	r := httptest.NewRequest("POST", "/", nil)
+	r.RemoteAddr = "192.168.50.10:51000"
+	r.Header.Set("Cf-Connecting-Ip", "203.0.113.77")
+	if got := clientIP(r); got != "203.0.113.77" {
+		t.Fatalf("clientIP = %q, want 203.0.113.77", got)
+	}
+	// outside the custom list
+	r2 := httptest.NewRequest("POST", "/", nil)
+	r2.RemoteAddr = "10.42.0.7:51000"
+	r2.Header.Set("Cf-Connecting-Ip", "203.0.113.77")
+	if got := clientIP(r2); got != "10.42.0.7" {
+		t.Fatalf("clientIP = %q, want peer 10.42.0.7 (outside custom trusted list)", got)
+	}
+}
+
+// IPv6 peer without port must not be mangled.
+func TestHostOnly(t *testing.T) {
+	cases := map[string]string{
+		"198.51.100.5:51000": "198.51.100.5",
+		"[2001:db8::1]:8080": "2001:db8::1",
+		"198.51.100.5":       "198.51.100.5",
+	}
+	for in, want := range cases {
+		if got := hostOnly(in); got != want {
+			t.Errorf("hostOnly(%q) = %q, want %q", in, got, want)
 		}
 	}
 }