Gate forwarded-header trust behind trusted-proxy config (closes #88)

All three rate limiters (receiver, login, password change) now key on the connection's own address unless the direct peer is inside the new TRUSTED_PROXIES CIDR list, in which case X-Forwarded-For is walked right to left for the first non-proxy hop. Default is the empty list, which trusts nothing. A set-but-unparseable value aborts startup.

internal/middleware/ratelimit.go

@@ -2,6 +2,9 @@ package middleware
 
 import (
 	"net/http"
+	"net/netip"
+	"slices"
+	"strings"
 	"time"
 
 	"github.com/go-chi/httprate"
@@ -31,13 +34,120 @@ const (
 	receiverRateInterval = 1 * time.Minute
 )
 
+// normalizeAddr strips the IPv4-in-IPv6 wrapper and any zone from
+// addr so that comparisons and bucket keys are canonical.
+func normalizeAddr(addr netip.Addr) netip.Addr {
+	return addr.Unmap().WithZone("")
+}
+
+// isTrustedProxy reports whether addr belongs to a network the
+// operator listed in TRUSTED_PROXIES. The list is empty by default,
+// so by default nothing is trusted.
+func (m *Middleware) isTrustedProxy(addr netip.Addr) bool {
+	for _, prefix := range m.params.Config.TrustedProxies {
+		if prefix.Contains(addr) {
+			return true
+		}
+	}
+
+	return false
+}
+
+// forwardedClientAddr returns the client address named by this
+// request's X-Forwarded-For chain. It is consulted only for requests
+// whose direct peer is a trusted proxy.
+//
+// X-Forwarded-For is the only header read. X-Real-IP and
+// True-Client-IP are deliberately ignored: the reverse proxies in
+// common use append to X-Forwarded-For and pass any other header the
+// client sent through untouched, so believing a single-valued header
+// would let a client behind the trusted proxy name its own bucket —
+// the very bypass this gating exists to close.
+//
+// The chain is walked right to left, because the rightmost entry is
+// the one the nearest proxy appended and everything to its left may
+// have been written by the client. The first hop that is not itself
+// a trusted proxy is the client. A hop that cannot be read as a bare
+// address ends the walk: past it the chain is not the shape assumed
+// here, so the caller falls back to the peer address.
+func (m *Middleware) forwardedClientAddr(
+	r *http.Request,
+) (netip.Addr, bool) {
+	hops := strings.Split(
+		strings.Join(r.Header.Values("X-Forwarded-For"), ","), ",",
+	)
+
+	for _, hop := range slices.Backward(hops) {
+		hop = strings.TrimSpace(hop)
+		if hop == "" {
+			continue
+		}
+
+		addr, err := netip.ParseAddr(hop)
+		if err != nil {
+			return netip.Addr{}, false
+		}
+
+		if addr = normalizeAddr(addr); !m.isTrustedProxy(addr) {
+			return addr, true
+		}
+	}
+
+	return netip.Addr{}, false
+}
+
+// rateLimitKey is the client identity every rate limiter in this
+// package buckets on. Forwarded headers are honoured only when the
+// direct peer (RemoteAddr) is inside the configured trusted-proxy
+// set; otherwise the peer address itself is the key. Without that
+// gate any client could mint a fresh bucket per request, or starve
+// another client's bucket, by picking an X-Forwarded-For value —
+// which makes every limit here decorative against a deliberate
+// attacker.
+func (m *Middleware) rateLimitKey(r *http.Request) (string, error) {
+	return m.clientKey(r), nil
+}
+
+// clientKey computes the bucket key described on rateLimitKey.
+func (m *Middleware) clientKey(r *http.Request) string {
+	peer, err := netip.ParseAddr(ipFromHostPort(r.RemoteAddr))
+	if err != nil {
+		// Not an address we can reason about; key on the raw
+		// value rather than collapsing such peers into one
+		// shared bucket.
+		return r.RemoteAddr
+	}
+
+	peer = normalizeAddr(peer)
+	if !m.isTrustedProxy(peer) {
+		return peer.String()
+	}
+
+	if addr, ok := m.forwardedClientAddr(r); ok {
+		return addr.String()
+	}
+
+	return peer.String()
+}
+
+// tooManyRequests returns the 429 handler shared by every limiter:
+// it logs the rejection with logMessage and answers with
+// responseMessage. httprate adds the Retry-After header (RFC 6585).
+func (m *Middleware) tooManyRequests(
+	logMessage, responseMessage string,
+) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		m.log.Warn(logMessage, "path", r.URL.Path)
+		http.Error(w, responseMessage, http.StatusTooManyRequests)
+	}
+}
+
 // LoginRateLimit returns middleware that enforces per-IP rate
 // limiting on login attempts using go-chi/httprate. Only POST
 // requests are rate-limited; GET requests (rendering the login
 // form) pass through unaffected. When the rate limit is exceeded,
-// a 429 Too Many Requests response is returned. IP extraction
-// honours X-Forwarded-For, X-Real-IP, and True-Client-IP headers
-// for reverse-proxy setups.
+// a 429 Too Many Requests response is returned. Clients are
+// identified by rateLimitKey.
 func (m *Middleware) LoginRateLimit() func(http.Handler) http.Handler {
 	return m.postRateLimit(
 		loginRateLimit,
@@ -66,9 +176,7 @@ func (m *Middleware) PasswordChangeRateLimit() func(http.Handler) http.Handler {
 // limit on POST requests only; all other methods pass through
 // unaffected. Requests over the limit receive a 429 with the
 // given response message, and each rejection is logged with the
-// given log message. IP extraction honours X-Forwarded-For,
-// X-Real-IP, and True-Client-IP headers for reverse-proxy
-// setups.
+// given log message. Clients are identified by rateLimitKey.
 func (m *Middleware) postRateLimit(
 	limit int,
 	interval time.Duration,
@@ -77,19 +185,10 @@ func (m *Middleware) postRateLimit(
 	limiter := httprate.Limit(
 		limit,
 		interval,
-		httprate.WithKeyFuncs(httprate.KeyByRealIP),
-		httprate.WithLimitHandler(http.HandlerFunc(
-			func(w http.ResponseWriter, r *http.Request) {
-				m.log.Warn(logMessage,
-					"path", r.URL.Path,
-				)
-				http.Error(
-					w,
-					responseMessage,
-					http.StatusTooManyRequests,
-				)
-			},
-		)),
+		httprate.WithKeyFuncs(m.rateLimitKey),
+		httprate.WithLimitHandler(
+			m.tooManyRequests(logMessage, responseMessage),
+		),
 	)
 
 	return func(next http.Handler) http.Handler {
@@ -116,31 +215,19 @@ func (m *Middleware) postRateLimit(
 // path (the path contains the entrypoint UUID, so each sender
 // is limited per entrypoint without affecting other senders or
 // other entrypoints). The limit is Config.ReceiverRateLimit
-// requests per minute. Requests over the limit receive a 429;
-// httprate adds the Retry-After header (RFC 6585). IP
-// extraction honours X-Forwarded-For, X-Real-IP, and
-// True-Client-IP headers for reverse-proxy setups.
+// requests per minute. Requests over the limit receive a 429.
+// Clients are identified by rateLimitKey.
 func (m *Middleware) ReceiverRateLimit() func(http.Handler) http.Handler {
 	return httprate.Limit(
 		m.params.Config.ReceiverRateLimit,
 		receiverRateInterval,
 		httprate.WithKeyFuncs(
-			httprate.KeyByRealIP,
+			m.rateLimitKey,
 			httprate.KeyByEndpoint,
 		),
-		httprate.WithLimitHandler(http.HandlerFunc(
-			func(w http.ResponseWriter, r *http.Request) {
-				m.log.Warn(
-					"webhook receiver rate limit exceeded",
-					"path", r.URL.Path,
-				)
-				http.Error(
-					w,
-					"Too many requests. "+
-						"Please slow down.",
-					http.StatusTooManyRequests,
-				)
-			},
+		httprate.WithLimitHandler(m.tooManyRequests(
+			"webhook receiver rate limit exceeded",
+			"Too many requests. Please slow down.",
 		)),
 	)
 }

internal/middleware/ratelimit_test.go

@@ -2,9 +2,11 @@ package middleware_test
 
 import (
 	"context"
+	"fmt"
 	"log/slog"
 	"net/http"
 	"net/http/httptest"
+	"net/netip"
 	"os"
 	"testing"
 
@@ -182,11 +184,22 @@ func TestLoginRateLimit_IndependentPerIP(t *testing.T) {
 	)
 }
 
-// receiverLimitedHandler builds a ReceiverRateLimit-wrapped
-// handler with the given per-minute limit.
-func receiverLimitedHandler(
-	t *testing.T, limit int,
-) http.Handler {
+// okHandler is the terminal handler the limiter middleware wraps
+// in these tests: it answers 200 to anything that reaches it.
+func okHandler() http.Handler {
+	return http.HandlerFunc(
+		func(w http.ResponseWriter, _ *http.Request) {
+			w.WriteHeader(http.StatusOK)
+		},
+	)
+}
+
+// rateLimitMiddleware builds a Middleware around cfg, whose
+// TrustedProxies field is what the rate limit key function gates
+// forwarded-header trust on.
+func rateLimitMiddleware(
+	t *testing.T, cfg *config.Config,
+) *middleware.Middleware {
 	t.Helper()
 
 	log := slog.New(slog.NewTextHandler(
@@ -194,17 +207,53 @@ func receiverLimitedHandler(
 		&slog.HandlerOptions{Level: slog.LevelDebug},
 	))
 
-	m := middleware.NewForTest(
-		log,
-		&config.Config{ReceiverRateLimit: limit},
-		nil,
+	return middleware.NewForTest(log, cfg, nil)
+}
+
+// trustedProxies parses CIDR strings for a test Config.
+func trustedProxies(cidrs ...string) []netip.Prefix {
+	prefixes := make([]netip.Prefix, 0, len(cidrs))
+	for _, cidr := range cidrs {
+		prefixes = append(prefixes, netip.MustParsePrefix(cidr))
+	}
+
+	return prefixes
+}
+
+// postWithHeaders sends one POST to the handler from peer with the
+// given headers set and returns the recorder.
+func postWithHeaders(
+	handler http.Handler,
+	peer, path string,
+	headers map[string]string,
+) *httptest.ResponseRecorder {
+	req := httptest.NewRequestWithContext(
+		context.Background(), http.MethodPost, path, nil,
+	)
+	req.RemoteAddr = peer
+
+	for name, value := range headers {
+		req.Header.Set(name, value)
+	}
+
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, req)
+
+	return w
+}
+
+// receiverLimitedHandler builds a ReceiverRateLimit-wrapped
+// handler with the given per-minute limit and no trusted proxies.
+func receiverLimitedHandler(
+	t *testing.T, limit int,
+) http.Handler {
+	t.Helper()
+
+	m := rateLimitMiddleware(
+		t, &config.Config{ReceiverRateLimit: limit},
 	)
 
-	return m.ReceiverRateLimit()(http.HandlerFunc(
-		func(w http.ResponseWriter, _ *http.Request) {
-			w.WriteHeader(http.StatusOK)
-		},
-	))
+	return m.ReceiverRateLimit()(okHandler())
 }
 
 // receiverPost sends one POST to the handler from the given IP
@@ -311,3 +360,252 @@ func TestReceiverRateLimit_CountsEveryMethod(t *testing.T) {
 		"a GET over the limit must be rate-limited",
 	)
 }
+
+const (
+	loginPath  = "/pages/login"
+	headerXFF  = "X-Forwarded-For"
+	headerReal = "X-Real-IP"
+	headerTrue = "True-Client-IP"
+)
+
+// assertSharedBucket drives the login limiter from peer with the
+// trusted-proxy set proxies, sending one more request than the limit
+// allows and varying the headers on each with headers(i). Every
+// request must land in the same bucket, so the last one is rejected:
+// if any of the varying header values reached the key, the run would
+// have minted fresh buckets and nothing would be rejected.
+func assertSharedBucket(
+	t *testing.T,
+	proxies []netip.Prefix,
+	peer string,
+	headers func(i int) map[string]string,
+	msg string,
+) {
+	t.Helper()
+
+	m := rateLimitMiddleware(
+		t, &config.Config{TrustedProxies: proxies},
+	)
+	handler := m.LoginRateLimit()(okHandler())
+
+	for i := range middleware.LoginRateLimitConst {
+		w := postWithHeaders(handler, peer, loginPath, headers(i))
+		assert.Equal(
+			t, http.StatusOK, w.Code,
+			"request %d should pass", i,
+		)
+	}
+
+	w := postWithHeaders(
+		handler, peer, loginPath,
+		headers(middleware.LoginRateLimitConst),
+	)
+	assert.Equal(t, http.StatusTooManyRequests, w.Code, msg)
+}
+
+// TestRateLimitKey_SpoofedForwardedFromUntrustedPeer is the test
+// this gating exists for: with no trusted proxies configured (the
+// default), a client that rotates a forwarded header on every
+// request must stay in one bucket. If forwarded headers were
+// trusted unconditionally, each spoofed value would mint a fresh
+// bucket and the limit would stop no one.
+func TestRateLimitKey_SpoofedForwardedFromUntrustedPeer(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	for _, header := range []string{
+		headerXFF, headerReal, headerTrue,
+	} {
+		t.Run(header, func(t *testing.T) {
+			t.Parallel()
+
+			assertSharedBucket(
+				t, nil, "203.0.113.9:44444",
+				func(i int) map[string]string {
+					return map[string]string{
+						header: fmt.Sprintf(
+							"198.51.100.%d", i+1,
+						),
+					}
+				},
+				"a spoofed "+header+" from an untrusted peer "+
+					"must not mint a fresh bucket",
+			)
+		})
+	}
+}
+
+// TestRateLimitKey_SingleValuedHeadersIgnoredFromTrustedPeer is the
+// regression test for the bypass hiding inside the trusted case.
+// Real reverse proxies (nginx, HAProxy, Caddy, ALB) set only
+// X-Forwarded-For and pass every other client header through
+// verbatim, so a client behind the configured proxy can send its own
+// X-Real-IP or True-Client-IP. Reading either would hand that client
+// a fresh bucket per request from inside exactly the deployment
+// TRUSTED_PROXIES exists to serve, so neither header is read at all.
+func TestRateLimitKey_SingleValuedHeadersIgnoredFromTrustedPeer(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	for _, header := range []string{headerReal, headerTrue} {
+		t.Run(header, func(t *testing.T) {
+			t.Parallel()
+
+			assertSharedBucket(
+				t, trustedProxies("10.0.0.0/8"),
+				"10.0.0.1:44444",
+				func(i int) map[string]string {
+					return map[string]string{
+						header: fmt.Sprintf(
+							"198.51.100.%d", i+1,
+						),
+					}
+				},
+				header+" from a trusted peer must not mint a "+
+					"fresh bucket: only X-Forwarded-For is read",
+			)
+		})
+	}
+}
+
+// TestRateLimitKey_MalformedRightmostHopFallsBackToPeer covers the
+// other end of the chain walk. The rightmost X-Forwarded-For entry
+// is the one the trusted proxy appended; if it cannot be read as an
+// address the chain is not the shape the walk assumes, and every
+// entry to its left may have come from the client. The walk must
+// stop and fall back to the peer rather than select one of them.
+func TestRateLimitKey_MalformedRightmostHopFallsBackToPeer(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	// Forms seen in the wild: host:port (Azure Application
+	// Gateway, IIS ARR), a bracketed IPv6 literal, and the
+	// RFC 7239 placeholder token.
+	for _, tail := range []string{
+		"198.51.100.7:1234", "[2001:db8::1]", "unknown",
+	} {
+		t.Run(tail, func(t *testing.T) {
+			t.Parallel()
+
+			assertSharedBucket(
+				t, trustedProxies("10.0.0.0/8"),
+				"10.0.0.1:44444",
+				func(i int) map[string]string {
+					return map[string]string{
+						headerXFF: fmt.Sprintf(
+							"9.9.9.%d, %s", i+1, tail,
+						),
+					}
+				},
+				"an unparseable rightmost hop must fall back "+
+					"to the peer address, not select a "+
+					"client-controlled entry",
+			)
+		})
+	}
+}
+
+// TestRateLimitKey_ForwardedHonouredFromTrustedPeer checks the
+// other half: when the direct peer is a configured trusted proxy,
+// the forwarded client address is what buckets are keyed on, so
+// one sender behind the proxy cannot exhaust another's limit.
+func TestRateLimitKey_ForwardedHonouredFromTrustedPeer(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	m := rateLimitMiddleware(t, &config.Config{
+		TrustedProxies: trustedProxies("10.0.0.0/8"),
+	})
+	handler := m.LoginRateLimit()(okHandler())
+
+	const peer = "10.0.0.1:44444"
+
+	first := map[string]string{headerXFF: "198.51.100.7"}
+
+	for range middleware.LoginRateLimitConst {
+		postWithHeaders(handler, peer, loginPath, first)
+	}
+
+	w := postWithHeaders(handler, peer, loginPath, first)
+	assert.Equal(
+		t, http.StatusTooManyRequests, w.Code,
+		"the forwarded client's own bucket must fill up",
+	)
+
+	w = postWithHeaders(
+		handler, peer, loginPath,
+		map[string]string{headerXFF: "198.51.100.8"},
+	)
+	assert.Equal(
+		t, http.StatusOK, w.Code,
+		"a forwarded header from a trusted peer must be honoured",
+	)
+}
+
+// TestRateLimitKey_ChainWalkSkipsClientPrepended covers the
+// residual spoofing route behind a trusted proxy: the client
+// controls the leftmost X-Forwarded-For entries, so the key is the
+// rightmost hop that is not itself trusted. Rotating the prepended
+// entry must not create new buckets.
+func TestRateLimitKey_ChainWalkSkipsClientPrepended(t *testing.T) {
+	t.Parallel()
+
+	assertSharedBucket(
+		t, trustedProxies("10.0.0.0/8"), "10.0.0.1:44444",
+		func(i int) map[string]string {
+			return map[string]string{
+				headerXFF: fmt.Sprintf(
+					"9.9.9.%d, 198.51.100.7, 10.0.0.2", i+1,
+				),
+			}
+		},
+		"a client-prepended X-Forwarded-For entry must not "+
+			"mint a fresh bucket",
+	)
+}
+
+// TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer proves
+// the receiver limiter uses the same gated key function as the
+// POST limiters.
+func TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	const (
+		limit = 3
+		peer  = "203.0.113.10:44444"
+		path  = "/webhook/uuid-d"
+	)
+
+	handler := receiverLimitedHandler(t, limit)
+
+	for i := range limit {
+		w := postWithHeaders(
+			handler, peer, path,
+			map[string]string{
+				headerXFF: fmt.Sprintf(
+					"198.51.100.%d", i+1,
+				),
+			},
+		)
+		assert.Equal(
+			t, http.StatusOK, w.Code,
+			"request %d should pass", i,
+		)
+	}
+
+	w := postWithHeaders(
+		handler, peer, path,
+		map[string]string{headerXFF: "198.51.100.200"},
+	)
+	assert.Equal(
+		t, http.StatusTooManyRequests, w.Code,
+		"a spoofed X-Forwarded-For from an untrusted peer must "+
+			"not mint a fresh receiver bucket",
+	)
+}