Honour forwarded headers only from trusted proxies (closes #259)

A new ClientIP middleware sets request.RemoteAddr to the client's
address before the request is logged. X-Forwarded-For and X-Real-IP
count only when the peer is in TRUSTED_PROXIES, which defaults to the
RFC 1918 ranges. The login rate limit, the session's IP address and
hostname, and the request log all use that address.

Model: opus-5-5

internal/config/config_test.go

@@ -237,21 +237,21 @@ func TestTrustedProxies(t *testing.T) {
 			set:       true,
 			value:     "198.51.100.0/24, 192.0.2.7,2001:db8::/32",
 			trusted:   []string{"198.51.100.200", "192.0.2.7", "2001:db8::5"},
-			untrusted: []string{"10.1.2.3", "192.168.1.1", "192.0.2.8"},
+			untrusted: []string{"10.4.5.6", "192.168.4.4", "192.0.2.8"},
 		},
 		{
 			name:      "a mapped IPv6 address is trusted as IPv4",
 			set:       true,
 			value:     "::ffff:192.0.2.7",
 			trusted:   []string{"192.0.2.7"},
-			untrusted: []string{"10.1.2.3"},
+			untrusted: []string{"10.7.8.9"},
 		},
 		{
 			name:      "the empty string trusts nothing",
 			set:       true,
 			value:     "",
 			trusted:   nil,
-			untrusted: []string{"10.1.2.3", "192.168.1.1", "127.0.0.1"},
+			untrusted: []string{"10.0.0.1", "192.168.0.1", "::1"},
 		},
 	}
 

internal/handlers/api_test.go

@@ -4295,7 +4295,7 @@ func TestLoginRateLimitIgnoresForwardedHeaders(t *testing.T) {
 	}
 
 	loginBody, err := json.Marshal(map[string]string{
-		nickKey: "nosuchuser", passwordKey: "doesnotmatter",
+		nickKey: "unknownnick", passwordKey: "wrongpassword",
 	})
 	if err != nil {
 		t.Fatal(err)

internal/middleware/middleware.go

@@ -6,6 +6,7 @@ import (
 	"net"
 	"net/http"
 	"net/netip"
+	"slices"
 	"strings"
 	"time"
 
@@ -119,11 +120,10 @@ func clientAddr(
 		return realIP.Unmap()
 	}
 
-	entries := strings.Split(forwarded, ",")
 	client := peer
 
-	for i := len(entries) - 1; i >= 0; i-- {
+	for _, entry := range slices.Backward(strings.Split(forwarded, ",")) {
-		addr, err := netip.ParseAddr(strings.TrimSpace(entries[i]))
+		addr, err := netip.ParseAddr(strings.TrimSpace(entry))
 		if err != nil {
 			return peer
 		}

internal/middleware/middleware_test.go

@@ -14,111 +14,122 @@ import (
 	"sneak.berlin/go/neoirc/internal/middleware"
 )
 
-func TestClientIP(t *testing.T) {
+// The tests trust 10.0.0.0/8. Clients use the documentation ranges
-	t.Parallel()
+// 192.0.2.0/24, 198.51.100.0/24 and 203.0.113.0/24.
+const (
+	trustedNetwork = "10.0.0.0/8"
+	proxyPeer      = "10.0.0.1:4000"
+	directPeer     = "192.0.2.1:4000"
+	client         = "203.0.113.7"
+	clientWithPort = "203.0.113.7:4000"
+	realIPClient   = "203.0.113.8"
+)
 
-	// The proxies are in 10.0.0.0/8. Clients use the documentation
+type clientIPTest struct {
-	// ranges 192.0.2.0/24, 198.51.100.0/24 and 203.0.113.0/24.
+	name       string
-	tests := []struct {
+	remoteAddr string
-		name       string
+	forwarded  []string
-		remoteAddr string
+	realIP     string
-		forwarded  []string
+	want       string
-		realIP     string
+}
-		want       string
+
-	}{
+func clientIPTests() []clientIPTest {
+	return []clientIPTest{
 		{
 			name:       "untrusted peer ignores forged headers",
-			remoteAddr: "192.0.2.1:4000",
+			remoteAddr: directPeer,
-			forwarded:  []string{"203.0.113.7"},
+			forwarded:  []string{client},
-			realIP:     "203.0.113.8",
+			realIP:     realIPClient,
-			want:       "192.0.2.1:4000",
+			want:       directPeer,
-		},
-		{
-			name:       "trusted peer without headers",
-			remoteAddr: "10.0.0.1:4000",
-			want:       "10.0.0.1:4000",
 		},
+		{name: "trusted peer without headers", remoteAddr: proxyPeer, want: proxyPeer},
 		{
 			name:       "trusted peer gives rightmost untrusted entry",
-			remoteAddr: "10.0.0.1:4000",
+			remoteAddr: proxyPeer,
 			forwarded:  []string{"198.51.100.9, 203.0.113.7, 10.0.0.2"},
-			want:       "203.0.113.7:4000",
+			want:       clientWithPort,
 		},
 		{
 			name:       "entries split over several headers",
-			remoteAddr: "10.0.0.1:4000",
+			remoteAddr: proxyPeer,
-			forwarded:  []string{"198.51.100.9", "203.0.113.7", "10.0.0.2"},
+			forwarded:  []string{"198.51.100.9", client, "10.0.0.2"},
-			want:       "203.0.113.7:4000",
+			want:       clientWithPort,
 		},
 		{
 			name:       "all-trusted chain gives leftmost entry",
-			remoteAddr: "10.0.0.1:4000",
+			remoteAddr: proxyPeer,
 			forwarded:  []string{"10.0.0.3, 10.0.0.2"},
 			want:       "10.0.0.3:4000",
 		},
 		{
 			name:       "X-Real-IP without X-Forwarded-For",
-			remoteAddr: "10.0.0.1:4000",
+			remoteAddr: proxyPeer,
-			realIP:     "203.0.113.8",
+			realIP:     realIPClient,
 			want:       "203.0.113.8:4000",
 		},
 		{
 			name:       "X-Forwarded-For wins over X-Real-IP",
-			remoteAddr: "10.0.0.1:4000",
+			remoteAddr: proxyPeer,
-			forwarded:  []string{"203.0.113.7"},
+			forwarded:  []string{client},
-			realIP:     "203.0.113.8",
+			realIP:     realIPClient,
-			want:       "203.0.113.7:4000",
+			want:       clientWithPort,
 		},
 		{
 			name:       "unparseable X-Forwarded-For entry gives peer",
-			remoteAddr: "10.0.0.1:4000",
+			remoteAddr: proxyPeer,
 			forwarded:  []string{"203.0.113.7, not-an-ip, 10.0.0.2"},
-			want:       "10.0.0.1:4000",
+			want:       proxyPeer,
 		},
 		{
 			name:       "unparseable entry past the client is not read",
-			remoteAddr: "10.0.0.1:4000",
+			remoteAddr: proxyPeer,
 			forwarded:  []string{"not-an-ip, 203.0.113.7"},
-			want:       "203.0.113.7:4000",
+			want:       clientWithPort,
 		},
 		{
 			name:       "unparseable X-Real-IP gives peer",
-			remoteAddr: "10.0.0.1:4000",
+			remoteAddr: proxyPeer,
 			realIP:     "not-an-ip",
-			want:       "10.0.0.1:4000",
+			want:       proxyPeer,
 		},
 		{
 			name:       "mapped IPv6 peer is unmapped and trusted",
 			remoteAddr: "[::ffff:10.0.0.1]:4000",
-			forwarded:  []string{"203.0.113.7"},
+			forwarded:  []string{client},
-			want:       "203.0.113.7:4000",
+			want:       clientWithPort,
 		},
 		{
 			name:       "mapped IPv6 peer outside the set",
 			remoteAddr: "[::ffff:192.0.2.1]:4000",
-			forwarded:  []string{"203.0.113.7"},
+			forwarded:  []string{client},
-			want:       "192.0.2.1:4000",
+			want:       directPeer,
 		},
 		{
 			name:       "IPv6 client",
-			remoteAddr: "10.0.0.1:4000",
+			remoteAddr: proxyPeer,
 			forwarded:  []string{"2001:db8::1"},
 			want:       "[2001:db8::1]:4000",
 		},
 	}
+}
 
-	handler := newMiddleware(t, "10.0.0.0/8").ClientIP()(
+func TestClientIP(t *testing.T) {
-		http.HandlerFunc(func(
+	t.Parallel()
-			writer http.ResponseWriter, request *http.Request,
-		) {
-			_, _ = writer.Write([]byte(request.RemoteAddr))
-		}),
-	)
 
-	for _, test := range tests {
+	mware := newMiddleware(t, trustedNetwork)
+
+	for _, test := range clientIPTests() {
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
 
+			var got string
+
+			handler := mware.ClientIP()(http.HandlerFunc(func(
+				_ http.ResponseWriter, request *http.Request,
+			) {
+				got = request.RemoteAddr
+			}))
+
 			request := httptest.NewRequestWithContext(
 				t.Context(), http.MethodGet, "/", nil,
 			)
@@ -132,10 +143,8 @@ func TestClientIP(t *testing.T) {
 				request.Header.Set("X-Real-IP", test.realIP)
 			}
 
-			recorder := httptest.NewRecorder()
+			handler.ServeHTTP(httptest.NewRecorder(), request)
-			handler.ServeHTTP(recorder, request)
 
-			got := recorder.Body.String()
 			if got != test.want {
 				t.Errorf("RemoteAddr = %q, want %q", got, test.want)
 			}