URL preview blacklisting fixes (#5155)

Prevents a SynapseError being raised inside of a IResolutionReceiver and instead opts to just return 0 results. This thus means that we have to lump a failed lookup and a blacklisted lookup together with the same error message, but the substitute should be generic enough to cover both cases.

changelog.d/5155.misc

@@ -0,0 +1 @@
+Prevent an exception from being raised in a IResolutionReceiver and use a more generic error message for blacklisted URL previews.

synapse/http/client.py

@@ -90,45 +90,50 @@ def __init__(self, reactor, ip_whitelist, ip_blacklist):
     def resolveHostName(self, recv, hostname, portNumber=0):
 
         r = recv()
-        d = defer.Deferred()
         addresses = []
 
-        @provider(IResolutionReceiver)
-        class EndpointReceiver(object):
-            @staticmethod
-            def resolutionBegan(resolutionInProgress):
-                pass
+        def _callback():
+            r.resolutionBegan(None)
 
-            @staticmethod
-            def addressResolved(address):
-                ip_address = IPAddress(address.host)
+            has_bad_ip = False
+            for i in addresses:
+                ip_address = IPAddress(i.host)
 
                 if check_against_blacklist(
                     ip_address, self._ip_whitelist, self._ip_blacklist
                 ):
                     logger.info(
-                        "Dropped %s from DNS resolution to %s" % (ip_address, hostname)
+                        "Dropped %s from DNS resolution to %s due to blacklist" %
+                        (ip_address, hostname)
                     )
-                    raise SynapseError(403, "IP address blocked by IP blacklist entry")
+                    has_bad_ip = True
+
+            # if we have a blacklisted IP, we'd like to raise an error to block the
+            # request, but all we can really do from here is claim that there were no
+            # valid results.
+            if not has_bad_ip:
+                for i in addresses:
+                    r.addressResolved(i)
+            r.resolutionComplete()
 
+        @provider(IResolutionReceiver)
+        class EndpointReceiver(object):
+            @staticmethod
+            def resolutionBegan(resolutionInProgress):
+                pass
+
+            @staticmethod
+            def addressResolved(address):
                 addresses.append(address)
 
             @staticmethod
             def resolutionComplete():
-                d.callback(addresses)
+                _callback()
 
         self._reactor.nameResolver.resolveHostName(
             EndpointReceiver, hostname, portNumber=portNumber
         )
 
-        def _callback(addrs):
-            r.resolutionBegan(None)
-            for i in addrs:
-                r.addressResolved(i)
-            r.resolutionComplete()
-
-        d.addCallback(_callback)
-
         return r
 
 

synapse/rest/media/v1/preview_url_resource.py

@@ -31,6 +31,7 @@
 from canonicaljson import json
 
 from twisted.internet import defer
+from twisted.internet.error import DNSLookupError
 from twisted.web.resource import Resource
 from twisted.web.server import NOT_DONE_YET
 
@@ -328,9 +329,18 @@ def _download_url(self, url, user):
                 # handler will return a SynapseError to the client instead of
                 # blank data or a 500.
                 raise
+            except DNSLookupError:
+                # DNS lookup returned no results
+                # Note: This will also be the case if one of the resolved IP
+                # addresses is blacklisted
+                raise SynapseError(
+                    502, "DNS resolution failure during URL preview generation",
+                    Codes.UNKNOWN
+                )
             except Exception as e:
                 # FIXME: pass through 404s and other error messages nicely
                 logger.warn("Error downloading %s: %r", url, e)
+
                 raise SynapseError(
                     500, "Failed to download content: %s" % (
                         traceback.format_exception_only(sys.exc_info()[0], e),

tests/rest/media/v1/test_url_preview.py

@@ -297,12 +297,12 @@ def test_blacklisted_ip_specific(self):
 
         # No requests made.
         self.assertEqual(len(self.reactor.tcpClients), 0)
-        self.assertEqual(channel.code, 403)
+        self.assertEqual(channel.code, 502)
         self.assertEqual(
             channel.json_body,
             {
                 'errcode': 'M_UNKNOWN',
-                'error': 'IP address blocked by IP blacklist entry',
+                'error': 'DNS resolution failure during URL preview generation',
             },
         )
 
@@ -318,12 +318,12 @@ def test_blacklisted_ip_range(self):
         request.render(self.preview_url)
         self.pump()
 
-        self.assertEqual(channel.code, 403)
+        self.assertEqual(channel.code, 502)
         self.assertEqual(
             channel.json_body,
             {
                 'errcode': 'M_UNKNOWN',
-                'error': 'IP address blocked by IP blacklist entry',
+                'error': 'DNS resolution failure during URL preview generation',
             },
         )
 
@@ -339,14 +339,14 @@ def test_blacklisted_ip_specific_direct(self):
 
         # No requests made.
         self.assertEqual(len(self.reactor.tcpClients), 0)
-        self.assertEqual(channel.code, 403)
         self.assertEqual(
             channel.json_body,
             {
                 'errcode': 'M_UNKNOWN',
                 'error': 'IP address blocked by IP blacklist entry',
             },
         )
+        self.assertEqual(channel.code, 403)
 
     def test_blacklisted_ip_range_direct(self):
         """
@@ -414,12 +414,12 @@ def test_blacklisted_ip_with_external_ip(self):
         )
         request.render(self.preview_url)
         self.pump()
-        self.assertEqual(channel.code, 403)
+        self.assertEqual(channel.code, 502)
         self.assertEqual(
             channel.json_body,
             {
                 'errcode': 'M_UNKNOWN',
-                'error': 'IP address blocked by IP blacklist entry',
+                'error': 'DNS resolution failure during URL preview generation',
             },
         )
 
@@ -439,12 +439,12 @@ def test_blacklisted_ipv6_specific(self):
 
         # No requests made.
         self.assertEqual(len(self.reactor.tcpClients), 0)
-        self.assertEqual(channel.code, 403)
+        self.assertEqual(channel.code, 502)
         self.assertEqual(
             channel.json_body,
             {
                 'errcode': 'M_UNKNOWN',
-                'error': 'IP address blocked by IP blacklist entry',
+                'error': 'DNS resolution failure during URL preview generation',
             },
         )
 
@@ -460,11 +460,11 @@ def test_blacklisted_ipv6_range(self):
         request.render(self.preview_url)
         self.pump()
 
-        self.assertEqual(channel.code, 403)
+        self.assertEqual(channel.code, 502)
         self.assertEqual(
             channel.json_body,
             {
                 'errcode': 'M_UNKNOWN',
-                'error': 'IP address blocked by IP blacklist entry',
+                'error': 'DNS resolution failure during URL preview generation',
             },
         )