use ipware

apps/codecov-api/codecov/settings_base.py

@@ -286,11 +286,37 @@
 
 # Session configuration
 
-# Controls how the X-Forwarded-For header is processed to get client IP addresses.
-# Set to how many trusted proxies you have in front of the application (must be
-# a positive integer). Defaults to trusting all proxies (this is unsafe unless
-# your ingress proxy strips X-Forwarded-For headers).
-TRUSTED_PROXY_DEPTH = get_config("setup", "http", "trusted_proxy_depth", default=0)
+# SECURITY: X-Forwarded-For header processing configuration
+# These settings control how client IP addresses are extracted from requests
+# that pass through reverse proxies or load balancers.
+
+# TRUSTED_PROXY_COUNT: Number of proxies in front of the application that append
+# to the X-Forwarded-For header. The library will traverse this many proxies from
+# the right side of the XFF chain to find the real client IP.
+# - Set to 0 (default) to only trust REMOTE_ADDR (safest for most setups)
+# - Set to N where N is the exact number of trusted proxies you control
+# Example: nginx -> load balancer -> app = set to 2
+TRUSTED_PROXY_COUNT = get_config("setup", "http", "trusted_proxy_count", default=0)
+
+# TRUSTED_PROXY_IPS: List of IP addresses of trusted proxies that are allowed
+# to set X-Forwarded-For headers. If specified, only requests from these IPs
+# will have their XFF headers processed.
+# Example: ["10.0.0.1", "192.168.1.1"]
+TRUSTED_PROXY_IPS = get_config("setup", "http", "trusted_proxy_ips", default=[])
+
+# Backward compatibility: Support old "trusted_proxy_depth" config key
+# DEPRECATED: Use trusted_proxy_count instead
+_legacy_depth = get_config("setup", "http", "trusted_proxy_depth", default=None)
+if _legacy_depth is not None:
+    import warnings
+
+    warnings.warn(
+        "Config key 'setup.http.trusted_proxy_depth' is deprecated. "
+        "Use 'setup.http.trusted_proxy_count' instead.",
+        DeprecationWarning,
+        stacklevel=2,
+    )
+    TRUSTED_PROXY_COUNT = _legacy_depth
 
 SESSION_COOKIE_DOMAIN = get_config(
     "setup", "http", "cookies_domain", default=".codecov.io"

apps/codecov-api/codecov_auth/helpers.py

@@ -5,6 +5,7 @@
 from django.contrib.admin.models import CHANGE, LogEntry
 from django.contrib.contenttypes.models import ContentType
 from django.http import HttpRequest
+from ipware import get_client_ip
 
 from codecov_auth.constants import GITLAB_BASE_URL
 
@@ -39,19 +40,30 @@ def get_client_ip_address(request: HttpRequest) -> str:
     Get the client's IP address from the request, parsing the X-Forwarded-For
     header if it exists with a configured proxy depth or Remote-Addr otherwise.
 
+    Uses django-ipware to safely extract the client IP from proxy headers,
+    with protection against header spoofing. The library uses the "right-most"
+    strategy, which means it trusts the rightmost non-trusted IP in the
+    X-Forwarded-For chain (the most reliable position).
+
     :param request: The HTTP request object
     :return: The client's IP address as a string
     """
-    x_forwarded_for: str | None = request.META.get("HTTP_X_FORWARDED_FOR")
-    if x_forwarded_for:
-        ips = x_forwarded_for.split(",")
-        depth = min(
-            settings.TRUSTED_PROXY_DEPTH, len(ips)
-        )  # Ensure we don't go out of bounds
-        ip = ips[-depth].strip()
-    else:
-        ip = request.META.get("REMOTE_ADDR")
-    return ip
+    # Get configuration for trusted proxies
+    proxy_count = getattr(settings, "TRUSTED_PROXY_COUNT", 0)
+    trusted_proxies = getattr(settings, "TRUSTED_PROXY_IPS", [])
+
+    # Use ipware to get the client IP with security best practices
+    client_ip, is_routable = get_client_ip(
+        request,
+        proxy_order="right-most",  # Use rightmost non-trusted IP (most secure)
+        proxy_count=proxy_count,  # Number of proxies to traverse
+        proxy_trusted_ips=trusted_proxies,  # List of trusted proxy IPs
+        request_header_order=["X_FORWARDED_FOR", "REMOTE_ADDR"],
+    )
+
+    # Fallback to REMOTE_ADDR if ipware couldn't determine the IP
+    # or return a safe default
+    return client_ip or request.META.get("REMOTE_ADDR", "unknown")
 
 
 # https://stackoverflow.com/questions/7905106/adding-a-log-entry-for-an-action-by-a-user-in-a-django-ap

apps/codecov-api/codecov_auth/tests/unit/test_helpers.py

@@ -40,56 +40,172 @@ def test_current_user_part_of_org_when_user_has_org():
     assert current_user_part_of_org(current_user, current_user) is True
 
 
-def test_client_ip_from_x_forwarded_for_default_depth():
+def test_client_ip_from_x_forwarded_for_default_count():
+    """
+    With default TRUSTED_PROXY_COUNT=0, should ignore XFF and use REMOTE_ADDR
+    for security (prevents XFF spoofing).
+    """
     request = Mock()
-    request.META = {"HTTP_X_FORWARDED_FOR": "127.0.0.1,blah", "REMOTE_ADDR": "lol"}
+    request.META = {"HTTP_X_FORWARDED_FOR": "127.0.0.1,blah", "REMOTE_ADDR": "10.0.0.1"}
 
     result = get_client_ip_address(request)
-    assert result == "127.0.0.1"
+    # Default count=0 means don't trust XFF, use REMOTE_ADDR
+    assert result == "10.0.0.1"
 
 
-@override_settings(TRUSTED_PROXY_DEPTH=2)
-def test_client_ip_from_x_forwarded_for_custom_depth():
+@override_settings(TRUSTED_PROXY_COUNT=1)
+def test_client_ip_from_x_forwarded_for_single_proxy():
+    """
+    With TRUSTED_PROXY_COUNT=1, trust one proxy and get the client IP
+    from the rightmost non-proxy IP in the XFF chain.
+    XFF format: client_ip, proxy_ip
+    """
     request = Mock()
     request.META = {
-        "HTTP_X_FORWARDED_FOR": "attacker_spoof,real_ip,another_ip",
-        "REMOTE_ADDR": "lol",
+        "HTTP_X_FORWARDED_FOR": "192.168.1.100, 10.0.0.1",
+        "REMOTE_ADDR": "10.0.0.1",
     }
 
     result = get_client_ip_address(request)
-    assert result == "real_ip"
+    # With 1 proxy, ipware returns the client IP (leftmost)
+    assert result == "192.168.1.100"
 
 
-@override_settings(TRUSTED_PROXY_DEPTH=5)
-def test_client_ip_from_x_forwarded_for_overflow_depth():
+@override_settings(TRUSTED_PROXY_COUNT=2)
+def test_client_ip_from_x_forwarded_for_multiple_proxies():
+    """
+    With TRUSTED_PROXY_COUNT=2, trust two proxies in the chain.
+    XFF format: client_ip, proxy1_ip, proxy2_ip
+    """
     request = Mock()
     request.META = {
-        "HTTP_X_FORWARDED_FOR": "attacker_spoof,real_ip,another_ip",
-        "REMOTE_ADDR": "lol",
+        "HTTP_X_FORWARDED_FOR": "203.0.113.1,192.168.1.1,10.0.0.1",
+        "REMOTE_ADDR": "10.0.0.1",
     }
 
     result = get_client_ip_address(request)
-    assert result == "attacker_spoof"
+    # With 2 proxies, ipware returns the real client IP
+    assert result == "203.0.113.1"
 
 
-@override_settings(TRUSTED_PROXY_DEPTH=1)
-def test_client_ip_from_x_forwarded_for_whitespace():
+@override_settings(TRUSTED_PROXY_COUNT=1)
+def test_client_ip_handles_whitespace_in_xff():
+    """
+    Should handle whitespace in X-Forwarded-For header correctly.
+    """
     request = Mock()
     request.META = {
-        "HTTP_X_FORWARDED_FOR": "attacker_spoof,real_ip, another_ip ",
-        "REMOTE_ADDR": "lol",
+        "HTTP_X_FORWARDED_FOR": "  192.168.1.100  ,   10.0.0.1  ",
+        "REMOTE_ADDR": "10.0.0.1",
     }
 
     result = get_client_ip_address(request)
-    assert result == "another_ip"
+    # ipware should handle whitespace and extract the client IP
+    assert result == "192.168.1.100"
+
+
+@override_settings(TRUSTED_PROXY_COUNT=2, TRUSTED_PROXY_IPS=["10.0.0.1", "10.0.0.2"])
+def test_client_ip_with_trusted_proxy_ips():
+    """
+    When TRUSTED_PROXY_IPS is set, only requests from those IPs
+    should have their XFF headers processed.
+    """
+    request = Mock()
+    request.META = {
+        "HTTP_X_FORWARDED_FOR": "203.0.113.1,192.168.1.1,10.0.0.1",
+        "REMOTE_ADDR": "10.0.0.1",
+    }
+
+    result = get_client_ip_address(request)
+    # Should trust the XFF because REMOTE_ADDR is in TRUSTED_PROXY_IPS
+    assert result == "203.0.113.1"
 
 
 def test_client_ip_from_remote_addr():
+    """
+    When no X-Forwarded-For header is present, should use REMOTE_ADDR.
+    """
+    request = Mock()
+    request.META = {"HTTP_X_FORWARDED_FOR": None, "REMOTE_ADDR": "10.0.0.1"}
+
+    result = get_client_ip_address(request)
+    assert result == "10.0.0.1"
+
+
+def test_client_ip_with_empty_xff():
+    """
+    When X-Forwarded-For is empty, should fall back to REMOTE_ADDR.
+    """
+    request = Mock()
+    request.META = {"HTTP_X_FORWARDED_FOR": "", "REMOTE_ADDR": "10.0.0.1"}
+
+    result = get_client_ip_address(request)
+    assert result == "10.0.0.1"
+
+
+def test_client_ip_prevents_xff_spoofing_with_default_config():
+    """
+    SECURITY TEST: With default config (TRUSTED_PROXY_COUNT=0),
+    should NOT trust X-Forwarded-For headers, preventing IP spoofing.
+    This is the safest default configuration.
+    """
+    request = Mock()
+    # Attacker tries to spoof their IP using XFF header
+    request.META = {
+        "HTTP_X_FORWARDED_FOR": "1.2.3.4,5.6.7.8,9.10.11.12",
+        "REMOTE_ADDR": "203.0.113.50",  # Real IP
+    }
+
+    result = get_client_ip_address(request)
+    # Should use REMOTE_ADDR, not the spoofed IPs
+    assert result == "203.0.113.50"
+
+
+@override_settings(TRUSTED_PROXY_COUNT=1)
+def test_client_ip_rightmost_strategy_prevents_leftmost_spoofing():
+    """
+    SECURITY TEST: Using right-most strategy prevents attackers from
+    spoofing IPs by prepending fake IPs to the XFF chain.
+    """
+    request = Mock()
+    # Attacker tries to spoof by prepending fake IPs
+    request.META = {
+        "HTTP_X_FORWARDED_FOR": "1.1.1.1,2.2.2.2,3.3.3.3,203.0.113.50",
+        "REMOTE_ADDR": "10.0.0.1",
+    }
+
+    result = get_client_ip_address(request)
+    # With 1 proxy, should get the IP just before the proxy (rightmost non-proxy)
+    # ipware will extract 3.3.3.3, not the leftmost spoofed IPs
+    assert result == "3.3.3.3"
+
+
+@override_settings(TRUSTED_PROXY_COUNT=2)
+def test_client_ip_with_insufficient_proxies_in_chain():
+    """
+    When TRUSTED_PROXY_COUNT is higher than the number of IPs in XFF,
+    should handle gracefully.
+    """
+    request = Mock()
+    request.META = {
+        "HTTP_X_FORWARDED_FOR": "192.168.1.100",  # Only 1 IP
+        "REMOTE_ADDR": "10.0.0.1",
+    }
+
+    result = get_client_ip_address(request)
+    # Should still return a valid IP (fallback behavior)
+    assert result in ["192.168.1.100", "10.0.0.1"]
+
+
+def test_client_ip_with_no_meta():
+    """
+    When request.META has neither XFF nor REMOTE_ADDR, should return 'unknown'.
+    """
     request = Mock()
-    request.META = {"HTTP_X_FORWARDED_FOR": None, "REMOTE_ADDR": "lol"}
+    request.META = {}
 
     result = get_client_ip_address(request)
-    assert result == "lol"
+    assert result == "unknown"
 
 
 @pytest.mark.django_db

apps/codecov-api/graphql_api/tests/test_views.py

@@ -264,21 +264,28 @@ def test_rate_limit_disabled(self):
         result = view._check_ratelimit(request)
         assert result == False
 
-    def test_client_ip_from_x_forwarded_for(self):
+    def test_client_ip_from_x_forwarded_for_default_config(self):
+        """
+        With default TRUSTED_PROXY_COUNT=0, XFF should be ignored for security.
+        """
         view = AsyncGraphqlView()
         request = Mock()
-        request.META = {"HTTP_X_FORWARDED_FOR": "127.0.0.1,blah", "REMOTE_ADDR": "lol"}
+        request.META = {
+            "HTTP_X_FORWARDED_FOR": "127.0.0.1,blah",
+            "REMOTE_ADDR": "10.0.0.1",
+        }
 
         result = view.get_client_ip(request)
-        assert result == "127.0.0.1"
+        # Default config ignores XFF to prevent spoofing
+        assert result == "10.0.0.1"
 
     def test_client_ip_from_remote_addr(self):
         view = AsyncGraphqlView()
         request = Mock()
-        request.META = {"HTTP_X_FORWARDED_FOR": None, "REMOTE_ADDR": "lol"}
+        request.META = {"HTTP_X_FORWARDED_FOR": None, "REMOTE_ADDR": "10.0.0.1"}
 
         result = view.get_client_ip(request)
-        assert result == "lol"
+        assert result == "10.0.0.1"
 
     async def test_required_variable_present(self):
         schema = generate_schema_with_required_variables()

pyproject.toml

@@ -69,6 +69,7 @@ codecov-api = [
     "django-csp==3.8.0",
     "django-cursor-pagination>=0.3.0",
     "django-filter>=2.4.0",
+    "django-ipware>=6.0.5",
     "djangorestframework>=3.15.2",
     "drf-spectacular-sidecar>=2023.3.1",
     "drf-spectacular>=0.26.2",