Merge pull request 'ratelimit: rightmost untrusted-hop-safe client IP (fixes #85)' (#89) from fix-85-xff into main

internal/api/ratelimit.go

@@ -48,8 +48,28 @@ func (l *limiter) allow(key string, rate, burst float64) bool {
 	return true
 }
 
-// clientIP extracts the request IP (no reverse proxy header by default).
+// clientIP extracts the client IP for rate-limit keying (#85).
+//
+// Trust boundary: palette runs behind exactly ONE trusted reverse proxy
+// (Traefik in the k3s pod network). Traefik APPENDS the real client IP to
+// X-Forwarded-For, so the RIGHTMOST entry is the last value the trusted
+// proxy observed and is unspoofable by the client (a client-supplied fake
+// entry only lands on the LEFT and is ignored). This matches chi's
+// middleware.RealIP semantics for a single trusted proxy hop.
+//
+// Direct connections (no XFF header) fall back to RemoteAddr. Directly
+// reachable deployments must NOT expose the app to untrusted networks
+// without a proxy in front, or attackers could forge the rightmost entry.
 func clientIP(r *http.Request) string {
+	if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
+		if i := strings.LastIndex(xff, ","); i >= 0 {
+			return strings.TrimSpace(xff[i+1:])
+		}
+		return strings.TrimSpace(xff)
+	}
+	if xr := r.Header.Get("X-Real-Ip"); xr != "" {
+		return strings.TrimSpace(xr)
+	}
 	host := r.RemoteAddr
 	if i := strings.LastIndex(host, ":"); i > 0 {
 		host = host[:i]

internal/api/ratelimit_xff_test.go

@@ -0,0 +1,85 @@
+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.
+
+import (
+	"bytes"
+	"net/http/httptest"
+	"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)
+	}
+}
+
+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) {
+	srv := newTestServer(t)
+	h := srv.routes()
+	for i := 0; i < 5; 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")
+		rr := httptest.NewRecorder()
+		h.ServeHTTP(rr, req)
+		if rr.Code != 201 {
+			t.Fatalf("req %d: want 201, got %d", i, rr.Code)
+		}
+	}
+	// 6th request, still the same real IP, new spoofed prefix: must 429
+	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")
+	rr := httptest.NewRecorder()
+	h.ServeHTTP(rr, req)
+	if rr.Code != 429 {
+		t.Fatalf("spoofed 6th req: want 429, got %d", rr.Code)
+	}
+}
+
+func spoofN(i int) string {
+	return "1.2.3." + string(rune('0'+i))
+}
+
+// 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)
+		}
+	}
+}