Merge pull request #5607 from dojutsu-user/remove-proxy-middleware

Remove ProxyMiddleware

Author: humitos

readthedocs/analytics/tests.py

@@ -1,7 +1,11 @@
 # -*- coding: utf-8 -*-
-from django.test import TestCase
+from django.test import TestCase, RequestFactory
 
-from .utils import anonymize_ip_address, anonymize_user_agent
+from .utils import (
+    anonymize_ip_address,
+    anonymize_user_agent,
+    get_client_ip,
+)
 
 
 class UtilsTests(TestCase):
@@ -28,3 +32,44 @@ def test_anonymize_ua(self):
             anonymize_user_agent('Some rare user agent'),
             'Rare user agent',
         )
+
+    def test_get_client_ip_with_x_forwarded_for(self):
+        
+        # only client's ip is present
+        request = RequestFactory().get('/')
+        request.META['HTTP_X_FORWARDED_FOR'] = '203.0.113.195'
+        client_ip = get_client_ip(request)
+        self.assertEqual(client_ip, '203.0.113.195')
+
+        # proxy1 and proxy2 are present along with client's ip
+        request = RequestFactory().get('/')
+        request.META['HTTP_X_FORWARDED_FOR'] = '203.0.113.195, 70.41.3.18, 150.172.238.178'
+        client_ip = get_client_ip(request)
+        self.assertEqual(client_ip, '203.0.113.195')
+
+        # client ip with port
+        request = RequestFactory().get('/')
+        request.META['HTTP_X_FORWARDED_FOR'] = '203.0.113.195:8080, 70.41.3.18, 150.172.238.178'
+        client_ip = get_client_ip(request)
+        self.assertEqual(client_ip, '203.0.113.195')
+
+        # client ip with port but not proxy1 and proxy2
+        request = RequestFactory().get('/')
+        request.META['HTTP_X_FORWARDED_FOR'] = '203.0.113.195:8080'
+        client_ip = get_client_ip(request)
+        self.assertEqual(client_ip, '203.0.113.195')
+
+        # no header is present
+        request = RequestFactory().get('/')
+        if request.META['REMOTE_ADDR']:
+            del request.META['REMOTE_ADDR']
+        client_ip = get_client_ip(request)
+        self.assertEqual(client_ip, None)
+
+    def test_get_client_ip_with_remote_addr(self):
+
+        request = RequestFactory().get('/')
+        self.assertIsNone(request.META.get('HTTP_X_FORWARDED_FOR'))
+        request.META['REMOTE_ADDR'] = '203.0.113.195'
+        client_ip = get_client_ip(request)
+        self.assertEqual(client_ip, '203.0.113.195')

readthedocs/analytics/utils.py

@@ -17,15 +17,26 @@
 
 
 def get_client_ip(request):
-    """Gets the real IP based on a request object."""
-    ip_address = request.META.get('REMOTE_ADDR')
+    """
+    Gets the real client's IP address.
 
-    # Get the original IP address (eg. "X-Forwarded-For: client, proxy1, proxy2")
-    x_forwarded_for = request.META.get('HTTP_X_FORWARDED_FOR', '').split(',')[0]
+    It returns the real IP address of the client based on ``HTTP_X_FORWARDED_FOR``
+    header. If ``HTTP_X_FORWARDED_FOR`` is not found, it returns the value of
+    ``REMOTE_ADDR`` header and returns ``None`` if both the headers are not found.
+    """
+    x_forwarded_for = request.META.get('HTTP_X_FORWARDED_FOR', None)
     if x_forwarded_for:
-        ip_address = x_forwarded_for.rsplit(':')[0]
+        # HTTP_X_FORWARDED_FOR can be a comma-separated list of IPs.
+        # The client's IP will be the first one.
+        # (eg. "X-Forwarded-For: client, proxy1, proxy2")
+        client_ip = x_forwarded_for.split(',')[0].strip()
+
+        # Removing the port number (if present)
+        client_ip = client_ip.rsplit(':')[0]
+    else:
+        client_ip = request.META.get('REMOTE_ADDR', None)
 
-    return ip_address
+    return client_ip
 
 
 def anonymize_ip_address(ip_address):

readthedocs/core/middleware.py

@@ -174,34 +174,6 @@ def process_request(self, request):
         return None
 
 
-# Forked from old Django
-class ProxyMiddleware(MiddlewareMixin):
-
-    """
-    Middleware that sets REMOTE_ADDR based on HTTP_X_FORWARDED_FOR, if the.
-
-    latter is set. This is useful if you're sitting behind a reverse proxy that
-    causes each request's REMOTE_ADDR to be set to 127.0.0.1. Note that this
-    does NOT validate HTTP_X_FORWARDED_FOR. If you're not behind a reverse proxy
-    that sets HTTP_X_FORWARDED_FOR automatically, do not use this middleware.
-    Anybody can spoof the value of HTTP_X_FORWARDED_FOR, and because this sets
-    REMOTE_ADDR based on HTTP_X_FORWARDED_FOR, that means anybody can "fake"
-    their IP address. Only use this when you can absolutely trust the value of
-    HTTP_X_FORWARDED_FOR.
-    """
-
-    def process_request(self, request):
-        try:
-            real_ip = request.META['HTTP_X_FORWARDED_FOR']
-        except KeyError:
-            return None
-        else:
-            # HTTP_X_FORWARDED_FOR can be a comma-separated list of IPs. The
-            # client's IP will be the first one.
-            real_ip = real_ip.split(',')[0].strip()
-            request.META['REMOTE_ADDR'] = real_ip
-
-
 class FooterNoSessionMiddleware(SessionMiddleware):
 
     """

readthedocs/settings/base.py

@@ -154,7 +154,6 @@ def USE_PROMOS(self):  # noqa
         return 'readthedocsext.donate' in self.INSTALLED_APPS
 
     MIDDLEWARE = (
-        'readthedocs.core.middleware.ProxyMiddleware',
         'readthedocs.core.middleware.FooterNoSessionMiddleware',
         'django.middleware.locale.LocaleMiddleware',
         'django.middleware.common.CommonMiddleware',