Fix tests and various small review issues

4 files changed, 37 insertions, 73 deletions

diff --git a/synapse/http/client.py b/synapse/http/client.py
index 368b0d49b7..77fe68818b 100644
--- a/synapse/http/client.py
+++ b/synapse/http/client.py
@@ -165,10 +165,10 @@ class BlacklistingAgentWrapper(Agent):
                 ip_address, self._ip_whitelist, self._ip_blacklist
             ):
                 logger.info(
-                    "Blocking access to %s because of blacklist. Returning 0 results" %
+                    "Blocking access to %s due to blacklist" %
                     (ip_address,)
                 )
-                e = SynapseError(404, "No results found")
+                e = SynapseError(403, "IP address blocked by IP blacklist entry")
                 return defer.fail(Failure(e))
         except Exception:
             # Not an IP
@@ -264,9 +264,6 @@ class SimpleHttpClient(object):
             uri (str): URI to query.
             data (bytes): Data to send in the request body, if applicable.
             headers (t.w.http_headers.Headers): Request headers.
-
-        Raises:
-            SynapseError: If the IP is blacklisted.
         """
         # A small wrapper around self.agent.request() so we can easily attach
         # counters to it
diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py
index 6fe337e4bb..7eefc7b1fc 100644
--- a/synapse/http/matrixfederationclient.py
+++ b/synapse/http/matrixfederationclient.py
@@ -199,7 +199,8 @@ class MatrixFederationHttpClient(object):
             tls_client_options_factory,
         )
 
-        # Prevent direct connections to blacklisted IP addresses
+        # Use a BlacklistingAgentWrapper to prevent circumventing the IP
+        # blacklist via IP literals in server names
         self.agent = BlacklistingAgentWrapper(
             self.agent, self.reactor,
             ip_blacklist=hs.config.federation_ip_range_blacklist,
diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py
index f1f24ebd1c..acf87709f2 100644
--- a/synapse/rest/media/v1/preview_url_resource.py
+++ b/synapse/rest/media/v1/preview_url_resource.py
@@ -329,18 +329,18 @@ class PreviewUrlResource(Resource):
                 # 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)
 
-                if isinstance(e, DNSLookupError):
-                    # DNS lookup returned no results
-                    # Note: This will also be the case if the found IP address
-                    # is blacklisted
-                    raise SynapseError(
-                        404, "No results found", Codes.UNKNOWN
-                    )
-
                 raise SynapseError(
                     500, "Failed to download content: %s" % (
                         traceback.format_exception_only(sys.exc_info()[0], e),
diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py
index 148e55b7a5..0a7e498182 100644
--- a/tests/http/test_fedclient.py
+++ b/tests/http/test_fedclient.py
@@ -228,93 +228,59 @@ class FederationClientTests(HomeserverTestCase):
 
         # Try making a GET request to a blacklisted IPv4 address
         # ------------------------------------------------------
-        @defer.inlineCallbacks
-        def do_request():
-            with LoggingContext("one") as context:
-                fetch_d = cl.get_json("internal:8008", "foo/bar")
-
-                # Nothing happened yet
-                self.assertNoResult(fetch_d)
-
-                # should have reset logcontext to the sentinel
-                check_logcontext(LoggingContext.sentinel)
-
-                try:
-                    fetch_res = yield fetch_d
-                    defer.returnValue(fetch_res)
-                finally:
-                    check_logcontext(context)
-
         # Make the request
-        d = do_request()
-        self.pump()
+        d = cl.get_json("internal:8008", "foo/bar", timeout=10000)
 
-        # Nothing has happened yet
+        # Nothing happened yet
         self.assertNoResult(d)
 
+        self.pump(120)
+
         # Check that it was unable to resolve the address
         clients = self.reactor.tcpClients
         self.assertEqual(len(clients), 0)
 
+        f = self.failureResultOf(d)
+        self.assertIsInstance(f.value, RequestSendFailed)
+        self.assertIsInstance(f.value.inner_exception, DNSLookupError)
+
         # Try making a POST request to a blacklisted IPv6 address
         # -------------------------------------------------------
-        @defer.inlineCallbacks
-        def do_request():
-            with LoggingContext("one") as context:
-                fetch_d = cl.post_json("internalv6:8008", "foo/bar")
-
-                # Nothing happened yet
-                self.assertNoResult(fetch_d)
-
-                # should have reset logcontext to the sentinel
-                check_logcontext(LoggingContext.sentinel)
-
-                try:
-                    fetch_res = yield fetch_d
-                    defer.returnValue(fetch_res)
-                finally:
-                    check_logcontext(context)
-
         # Make the request
-        d = do_request()
-        self.pump()
+        d = cl.post_json("internalv6:8008", "foo/bar", timeout=10000)
 
         # Nothing has happened yet
         self.assertNoResult(d)
 
+        # Move the reactor forwards
+        self.pump(120)
+
         # Check that it was unable to resolve the address
         clients = self.reactor.tcpClients
         self.assertEqual(len(clients), 0)
 
+        # Check that it was due to a blacklisted DNS lookup
+        f = self.failureResultOf(d, RequestSendFailed)
+        self.assertIsInstance(f.value.inner_exception, DNSLookupError)
+
         # Try making a GET request to a non-blacklisted IPv4 address
         # ----------------------------------------------------------
-        @defer.inlineCallbacks
-        def do_request():
-            with LoggingContext("one") as context:
-                fetch_d = cl.post_json("fine:8008", "foo/bar")
-
-                # Nothing happened yet
-                self.assertNoResult(fetch_d)
-
-                # should have reset logcontext to the sentinel
-                check_logcontext(LoggingContext.sentinel)
-
-                try:
-                    fetch_res = yield fetch_d
-                    defer.returnValue(fetch_res)
-                finally:
-                    check_logcontext(context)
-
         # Make the request
-        d = do_request()
-        self.pump()
+        d = cl.post_json("fine:8008", "foo/bar", timeout=10000)
 
         # Nothing has happened yet
         self.assertNoResult(d)
 
+        # Move the reactor forwards
+        self.pump(120)
+
         # Check that it was able to resolve the address
         clients = self.reactor.tcpClients
-        self.assertEqual(len(clients), 1)
+        self.assertNotEqual(len(clients), 0)
+
+        # Connection will still fail as this IP address does not resolve to anything
+        f = self.failureResultOf(d, RequestSendFailed)
+        self.assertIsInstance(f.value.inner_exception, ConnectingCancelledError)
 
     def test_client_gets_headers(self):
         """