From aee810a54812df76a78743a88772b1bb9063fcab Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 8 May 2019 11:52:25 -0700 Subject: [PATCH] Fix tests and various small review issues --- synapse/http/client.py | 7 +- synapse/http/matrixfederationclient.py | 3 +- synapse/rest/media/v1/preview_url_resource.py | 16 ++-- tests/http/test_fedclient.py | 84 ++++++------------- 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): """