From 3d1d5ee1fd6777e13b5891ccb1184b30c150729b Mon Sep 17 00:00:00 2001 From: agent Date: Wed, 9 Sep 2026 10:57:59 -0500 Subject: [PATCH] ratelimit: key on rightmost X-Forwarded-For entry (fixes #85) --- internal/api/ratelimit.go | 22 +++++++- internal/api/ratelimit_xff_test.go | 85 ++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 internal/api/ratelimit_xff_test.go diff --git a/internal/api/ratelimit.go b/internal/api/ratelimit.go index 17a5a8b..5533831 100644 --- a/internal/api/ratelimit.go +++ b/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] diff --git a/internal/api/ratelimit_xff_test.go b/internal/api/ratelimit_xff_test.go new file mode 100644 index 0000000..8674636 --- /dev/null +++ b/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) + } + } +} -- 2.54.0