Handle IPv6 in client_addr tween

ClientAddr tween was crashing when getting IPv6 addresses.

- Use IP version agnostic parsing.
- Fetch the list of CloudFlare's IPv6 IPs.

CHANGES.rst

@@ -2,6 +2,15 @@
 Changes
 =======
 
+0.9.2
+-----
+
+ * Handle IPv6 in client_addr tween. ClientAddr tween was crashing when getting
+   IPv6 addresses. Use IP version agnostic parsing. Fetch the list of
+   CloudFlare's IPv6 IPs.
+   [am-on]
+
+
 0.9.1
 -----
 

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "pyramid_heroku"
-version = "0.9.1"
+version = "0.9.2"
 description = "A bunch of helpers for successfully running Pyramid on Heroku."
 readme = "README.rst"
 include = [ "CHANGES.rst", "COPYING.txt" ]

pyramid_heroku/client_addr.py

@@ -1,6 +1,6 @@
 """Set client_addr IP that we can trust."""
-from ipaddress import IPv4Address
-from ipaddress import IPv4Network
+from ipaddress import ip_address
+from ipaddress import ip_network
 
 import logging
 import requests
@@ -41,6 +41,14 @@ class ClientAddr(object):
     def __init__(self, handler, registry):
         self.handler = handler
         self.registry = registry
+
+        if self.registry.settings.get("pyramid_heroku.structlog"):
+            import structlog
+
+            self.logger = structlog.getLogger(__name__)
+        else:
+            self.logger = logging.getLogger(__name__)
+
         self.ignored_ip_networks = self.get_cloudflare_ip_networks()
 
     def __call__(self, request):
@@ -56,26 +64,33 @@ def __call__(self, request):
     def remove_ignored_ips(self, ips):
         """Remove ignored IPs from the list."""
         for ip in ips:
+            try:
+                address = ip_address(ip)
+            except ValueError as e:
+                self.logger.info("Ignoring invalid IP address", exc_info=e)
+                continue
+
             is_ignored = any(
-                [IPv4Address(ip) in network for network in self.ignored_ip_networks]
+                [address in network for network in self.ignored_ip_networks]
             )
             if not is_ignored:
                 yield ip
 
-    def get_cloudflare_ip_networks(self):
-        """Get the list of Cloudflare's current IP ranges."""
+    def get_ip_networks_from_url(self, url):
+        """Get the list of IP networks from given url."""
         try:
-            res = requests.get("https://www.cloudflare.com/ips-v4", timeout=10)
+            res = requests.get(url, timeout=10)
             res.raise_for_status()
-            return [IPv4Network(n) for n in res.text.split()]
+            return [ip_network(n) for n in res.text.split()]
         except Exception as e:
-            if self.registry.settings.get("pyramid_heroku.structlog"):
-                import structlog
-
-                logger = structlog.getLogger(__name__)
-            else:
-
-                logger = logging.getLogger(__name__)
-            logger.exception("Failed getting a list of Cloudflare IPs", exc_info=e)
+            self.logger.exception(
+                f"Failed getting a list of IPs from {url}", exc_info=e
+            )
 
             return []
+
+    def get_cloudflare_ip_networks(self):
+        """Get the list of Cloudflare's current IP ranges."""
+        ipv4 = self.get_ip_networks_from_url("https://www.cloudflare.com/ips-v4")
+        ipv6 = self.get_ip_networks_from_url("https://www.cloudflare.com/ips-v6")
+        return ipv4 + ipv6

pyramid_heroku/tests/test_client_addr.py

@@ -26,16 +26,23 @@ def setUp(self):
         self.request = request.Request({})
 
         self.handler = mock.Mock()
-        self.registry = None
+        self.registry = mock.Mock()
+        self.registry.settings = {}
 
         self.responses = responses.RequestsMock()
         self.responses.start()
         self.responses.add(
             responses.GET,
-            "https://www.cloudflare.com/ips-v4",  # noqa
+            "https://www.cloudflare.com/ips-v4",
             status=200,
             body="8.8.8.0/24\n9.9.9.0/24",
         )
+        self.responses.add(
+            responses.GET,
+            "https://www.cloudflare.com/ips-v6",
+            status=200,
+            body="2400:cb00::/32\n2606:4700::/32",
+        )
 
         structlog.configure(processors=[self.wrap_logger], context_class=dict)
 
@@ -77,7 +84,45 @@ def test_cloudflare_ip_ignored(self):
         from pyramid_heroku.client_addr import ClientAddr
 
         self.request.environ["REMOTE_ADDR"] = "127.0.0.1"  # load balancer
-        self.request.headers["X-Forwarded-For"] = " 6.6.6.6, 1.2.3.4, 9.9.9.9"
+        self.request.headers["X-Forwarded-For"] = "6.6.6.6, 1.2.3.4, 9.9.9.9"
+
+        ClientAddr(self.handler, self.registry)(self.request)
+        self.handler.assert_called_with(self.request)
+        self.assertEqual(self.request.client_addr, "1.2.3.4")
+
+    def test_cloudflare_ipv6_ignored(self):
+        from pyramid_heroku.client_addr import ClientAddr
+
+        self.request.environ["REMOTE_ADDR"] = "127.0.0.1"  # load balancer
+        self.request.headers["X-Forwarded-For"] = (
+            "2600:cb00:0000:0000:0000:0000:0000:0001, "
+            "2500:cb00:0000:0000:0000:0000:0000:0001, "
+            "2400:cb00:0000:0000:0000:0000:0000:0001"
+        )
+
+        ClientAddr(self.handler, self.registry)(self.request)
+        self.handler.assert_called_with(self.request)
+        self.assertEqual(
+            self.request.client_addr, "2500:cb00:0000:0000:0000:0000:0000:0001"
+        )
+
+    def test_invalid_ips(self):
+        from pyramid_heroku.client_addr import ClientAddr
+
+        self.request.environ["REMOTE_ADDR"] = "127.0.0.1"  # load balancer
+        self.request.headers["X-Forwarded-For"] = "1.2.3.4, invalid_ip not_ip"
+
+        ClientAddr(self.handler, self.registry)(self.request)
+        self.handler.assert_called_with(self.request)
+        self.assertEqual(self.request.client_addr, "1.2.3.4")
+
+    def test_cloudflare_ipv4_6_ignored(self):
+        from pyramid_heroku.client_addr import ClientAddr
+
+        self.request.environ["REMOTE_ADDR"] = "127.0.0.1"  # load balancer
+        self.request.headers[
+            "X-Forwarded-For"
+        ] = "1.2.3.4, 9.9.9.9, 2400:cb00:0000:0000:0000:0000:0000:0001"
 
         ClientAddr(self.handler, self.registry)(self.request)
         self.handler.assert_called_with(self.request)
@@ -88,10 +133,10 @@ def test_cloudflare_ip_list_get_error(self):
 
         self.responses.reset()
         self.responses.add(
-            responses.GET,
-            "https://www.cloudflare.com/ips-v4",  # noqa
-            status=501,
-            body="error",
+            responses.GET, "https://www.cloudflare.com/ips-v4", status=501, body="error"
+        )
+        self.responses.add(
+            responses.GET, "https://www.cloudflare.com/ips-v6", status=501, body="error"
         )
 
         self.request.environ["REMOTE_ADDR"] = "127.0.0.1"  # load balancer
@@ -103,9 +148,14 @@ def test_cloudflare_ip_list_get_error(self):
         self.handler.assert_called_with(self.request)
         self.assertEqual(self.request.client_addr, "9.9.9.9")
 
-        self.assertEqual(len(tweens_handler.records), 1)
+        self.assertEqual(len(tweens_handler.records), 2)
         self.assertEqual(
-            "Failed getting a list of Cloudflare IPs", tweens_handler.records[0].msg
+            "Failed getting a list of IPs from https://www.cloudflare.com/ips-v4",
+            tweens_handler.records[0].msg,
+        )
+        self.assertEqual(
+            "Failed getting a list of IPs from https://www.cloudflare.com/ips-v6",
+            tweens_handler.records[1].msg,
         )
 
         # structlog logging
@@ -117,13 +167,21 @@ def test_cloudflare_ip_list_get_error(self):
             status=501,
             body="error",
         )
+        self.responses.add(
+            responses.GET, "https://www.cloudflare.com/ips-v6", status=501, body="error"
+        )
 
         registry.settings = {"pyramid_heroku.structlog": True}
         ClientAddr(self.handler, registry)(self.request)
         self.handler.assert_called_with(self.request)
         self.assertEqual(self.request.client_addr, "9.9.9.9")
 
-        self.assertEqual(len(tweens_handler.records), 1)
+        self.assertEqual(len(tweens_handler.records), 2)
+        self.assertEqual(
+            "Failed getting a list of IPs from https://www.cloudflare.com/ips-v4",
+            tweens_handler.records[0].msg,
+        )
         self.assertEqual(
-            "Failed getting a list of Cloudflare IPs", tweens_handler.records[0].msg
+            "Failed getting a list of IPs from https://www.cloudflare.com/ips-v6",
+            tweens_handler.records[1].msg,
         )