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/handlers/api.go

@@ -42,25 +42,9 @@ func (hdlr *Handlers) maxBodySize() int64 {
 	return defaultMaxBodySize
 }
 
-// clientIP extracts the connecting client's IP address
-// from the request, checking X-Forwarded-For and
-// X-Real-IP headers before falling back to RemoteAddr.
+// clientIP returns the IP address in request.RemoteAddr, which the
+// ClientIP middleware has set to the client's address.
 func clientIP(request *http.Request) string {
-	if forwarded := request.Header.Get("X-Forwarded-For"); forwarded != "" {
-		// X-Forwarded-For can contain a comma-separated list;
-		// the first entry is the original client.
-		parts := strings.SplitN(forwarded, ",", 2) //nolint:mnd
-		ip := strings.TrimSpace(parts[0])
-
-		if ip != "" {
-			return ip
-		}
-	}
-
-	if realIP := request.Header.Get("X-Real-IP"); realIP != "" {
-		return strings.TrimSpace(realIP)
-	}
-
 	host, _, err := net.SplitHostPort(request.RemoteAddr)
 	if err != nil {
 		return request.RemoteAddr

internal/handlers/api_test.go

@@ -4281,6 +4281,117 @@ func TestLoginRateLimitExceeded(t *testing.T) {
 	}
 }
 
+func TestLoginRateLimitIgnoresForwardedHeaders(t *testing.T) {
+	t.Parallel()
+
+	// The test client connects from 127.0.0.1, which is outside the
+	// default trusted proxies, so the server must ignore these headers.
+	tests := []struct {
+		name   string
+		header string
+	}{
+		{name: "X-Forwarded-For", header: "X-Forwarded-For"},
+		{name: "X-Real-IP", header: "X-Real-IP"},
+	}
+
+	loginBody, err := json.Marshal(map[string]string{
+		nickKey: "unknownnick", passwordKey: "wrongpassword",
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			tserver := newTestServer(t)
+
+			// The default burst is 5 attempts, so the sixth gets 429
+			// although each claims a new address.
+			var status int
+
+			for attempt := range 6 {
+				request, rErr := http.NewRequestWithContext(
+					t.Context(), http.MethodPost,
+					tserver.url("/api/v1/login"),
+					bytes.NewReader(loginBody),
+				)
+				if rErr != nil {
+					t.Fatal(rErr)
+				}
+
+				request.Header.Set("Content-Type", "application/json")
+				request.Header.Set(
+					test.header, fmt.Sprintf("203.0.113.%d", attempt+1),
+				)
+
+				resp, dErr := http.DefaultClient.Do(request)
+				if dErr != nil {
+					t.Fatal(dErr)
+				}
+
+				status = resp.StatusCode
+				_ = resp.Body.Close()
+			}
+
+			if status != http.StatusTooManyRequests {
+				t.Fatalf("sixth attempt got %d, want 429", status)
+			}
+		})
+	}
+}
+
+func TestLoginRateLimitPerClientBehindTrustedProxy(t *testing.T) {
+	// The test client connects from 127.0.0.1. Trusted as a proxy, it
+	// passes each client's address in X-Forwarded-For. t.Setenv rules
+	// out t.Parallel.
+	t.Setenv("TRUSTED_PROXIES", "127.0.0.1")
+
+	tserver := newTestServer(t)
+
+	// An empty JSON object is refused after the rate limit counts the
+	// attempt, so no attempt checks a password or looks up a hostname.
+	loginFrom := func(forwardedFor string) int {
+		t.Helper()
+
+		request, err := http.NewRequestWithContext(
+			t.Context(), http.MethodPost,
+			tserver.url("/api/v1/login"), strings.NewReader("{}"),
+		)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		request.Header.Set("Content-Type", "application/json")
+		request.Header.Set("X-Forwarded-For", forwardedFor)
+
+		resp, err := http.DefaultClient.Do(request)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		_ = resp.Body.Close()
+
+		return resp.StatusCode
+	}
+
+	// The default burst is 5 attempts per address.
+	for range 5 {
+		loginFrom("203.0.113.1")
+	}
+
+	status := loginFrom("203.0.113.1")
+	if status != http.StatusTooManyRequests {
+		t.Errorf("sixth attempt from 203.0.113.1 got %d, want 429", status)
+	}
+
+	status = loginFrom("203.0.113.2")
+	if status == http.StatusTooManyRequests {
+		t.Error("first attempt from 203.0.113.2 got 429")
+	}
+}
+
 func TestLoginRateLimitAllowsNormalUse(t *testing.T) {
 	t.Parallel()