Remove trailing slashes from outbound federation requests and retry on 400 (#4840)

As per #3622, we remove trailing slashes from outbound federation requests. However, to ensure that we remain backwards compatible with previous versions of Synapse, if we receive a HTTP 400 with `M_UNRECOGNIZED`, then we are likely talking to an older version of Synapse in which case we retry with a trailing slash appended to the request path.

changelog.d/4840.feature

@@ -0,0 +1 @@
+Remove trailing slashes from certain outbound federation requests. Retry if receiving a 404. Context: #3622.

synapse/federation/transport/client.py

@@ -51,9 +51,10 @@ class TransportLayerClient(object):
         logger.debug("get_room_state dest=%s, room=%s",
                      destination, room_id)
 
-        path = _create_v1_path("/state/%s/", room_id)
+        path = _create_v1_path("/state/%s", room_id)
         return self.client.get_json(
             destination, path=path, args={"event_id": event_id},
+            try_trailing_slash_on_400=True,
         )
 
     @log_function
@@ -73,9 +74,10 @@ class TransportLayerClient(object):
         logger.debug("get_room_state_ids dest=%s, room=%s",
                      destination, room_id)
 
-        path = _create_v1_path("/state_ids/%s/", room_id)
+        path = _create_v1_path("/state_ids/%s", room_id)
         return self.client.get_json(
             destination, path=path, args={"event_id": event_id},
+            try_trailing_slash_on_400=True,
         )
 
     @log_function
@@ -95,8 +97,11 @@ class TransportLayerClient(object):
         logger.debug("get_pdu dest=%s, event_id=%s",
                      destination, event_id)
 
-        path = _create_v1_path("/event/%s/", event_id)
+        path = _create_v1_path("/event/%s", event_id)
-        return self.client.get_json(destination, path=path, timeout=timeout)
+        return self.client.get_json(
+            destination, path=path, timeout=timeout,
+            try_trailing_slash_on_400=True,
+        )
 
     @log_function
     def backfill(self, destination, room_id, event_tuples, limit):
@@ -121,7 +126,7 @@ class TransportLayerClient(object):
             # TODO: raise?
             return
 
-        path = _create_v1_path("/backfill/%s/", room_id)
+        path = _create_v1_path("/backfill/%s", room_id)
 
         args = {
             "v": event_tuples,
@@ -132,6 +137,7 @@ class TransportLayerClient(object):
             destination,
             path=path,
             args=args,
+            try_trailing_slash_on_400=True,
         )
 
     @defer.inlineCallbacks
@@ -176,6 +182,7 @@ class TransportLayerClient(object):
             json_data_callback=json_data_callback,
             long_retries=True,
             backoff_on_404=True,  # If we get a 404 the other side has gone
+            try_trailing_slash_on_400=True,
         )
 
         defer.returnValue(response)
@@ -959,7 +966,7 @@ def _create_v1_path(path, *args):
 
     Example:
 
-        _create_v1_path("/event/%s/", event_id)
+        _create_v1_path("/event/%s", event_id)
 
     Args:
         path (str): String template for the path
@@ -980,7 +987,7 @@ def _create_v2_path(path, *args):
 
     Example:
 
-        _create_v2_path("/event/%s/", event_id)
+        _create_v2_path("/event/%s", event_id)
 
     Args:
         path (str): String template for the path

synapse/http/matrixfederationclient.py

@@ -188,6 +188,57 @@ class MatrixFederationHttpClient(object):
 
         self._cooperator = Cooperator(scheduler=schedule)
 
+    @defer.inlineCallbacks
+    def _send_request_with_optional_trailing_slash(
+        self,
+        request,
+        try_trailing_slash_on_400=False,
+        **send_request_args
+    ):
+        """Wrapper for _send_request which can optionally retry the request
+        upon receiving a combination of a 400 HTTP response code and a
+        'M_UNRECOGNIZED' errcode. This is a workaround for Synapse <= v0.99.3
+        due to #3622.
+
+        Args:
+            request (MatrixFederationRequest): details of request to be sent
+            try_trailing_slash_on_400 (bool): Whether on receiving a 400
+                'M_UNRECOGNIZED' from the server to retry the request with a
+                trailing slash appended to the request path.
+            send_request_args (Dict): A dictionary of arguments to pass to
+                `_send_request()`.
+
+        Raises:
+            HttpResponseException: If we get an HTTP response code >= 300
+                (except 429).
+
+        Returns:
+            Deferred[Dict]: Parsed JSON response body.
+        """
+        try:
+            response = yield self._send_request(
+                request, **send_request_args
+            )
+        except HttpResponseException as e:
+            # Received an HTTP error > 300. Check if it meets the requirements
+            # to retry with a trailing slash
+            if not try_trailing_slash_on_400:
+                raise
+
+            if e.code != 400 or e.to_synapse_error().errcode != "M_UNRECOGNIZED":
+                raise
+
+            # Retry with a trailing slash if we received a 400 with
+            # 'M_UNRECOGNIZED' which some endpoints can return when omitting a
+            # trailing slash on Synapse <= v0.99.3.
+            request.path += "/"
+
+            response = yield self._send_request(
+                request, **send_request_args
+            )
+
+        defer.returnValue(response)
+
     @defer.inlineCallbacks
     def _send_request(
         self,
@@ -196,7 +247,7 @@ class MatrixFederationHttpClient(object):
         timeout=None,
         long_retries=False,
         ignore_backoff=False,
-        backoff_on_404=False
+        backoff_on_404=False,
     ):
         """
         Sends a request to the given server.
@@ -473,7 +524,8 @@ class MatrixFederationHttpClient(object):
                  json_data_callback=None,
                  long_retries=False, timeout=None,
                  ignore_backoff=False,
-                 backoff_on_404=False):
+                 backoff_on_404=False,
+                 try_trailing_slash_on_400=False):
         """ Sends the specifed json data using PUT
 
         Args:
@@ -493,7 +545,12 @@ class MatrixFederationHttpClient(object):
                 and try the request anyway.
             backoff_on_404 (bool): True if we should count a 404 response as
                 a failure of the server (and should therefore back off future
-                requests)
+                requests).
+            try_trailing_slash_on_400 (bool): True if on a 400 M_UNRECOGNIZED
+                response we should try appending a trailing slash to the end
+                of the request. Workaround for #3622 in Synapse <= v0.99.3. This
+                will be attempted before backing off if backing off has been
+                enabled.
 
         Returns:
             Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The
@@ -509,7 +566,6 @@ class MatrixFederationHttpClient(object):
             RequestSendFailed: If there were problems connecting to the
                 remote, due to e.g. DNS failures, connection timeouts etc.
         """
-
         request = MatrixFederationRequest(
             method="PUT",
             destination=destination,
@@ -519,17 +575,19 @@ class MatrixFederationHttpClient(object):
             json=data,
         )
 
-        response = yield self._send_request(
+        response = yield self._send_request_with_optional_trailing_slash(
             request,
+            try_trailing_slash_on_400,
+            backoff_on_404=backoff_on_404,
+            ignore_backoff=ignore_backoff,
             long_retries=long_retries,
             timeout=timeout,
-            ignore_backoff=ignore_backoff,
-            backoff_on_404=backoff_on_404,
         )
 
         body = yield _handle_json_response(
             self.hs.get_reactor(), self.default_timeout, request, response,
         )
+
         defer.returnValue(body)
 
     @defer.inlineCallbacks
@@ -592,7 +650,8 @@ class MatrixFederationHttpClient(object):
 
     @defer.inlineCallbacks
     def get_json(self, destination, path, args=None, retry_on_dns_fail=True,
-                 timeout=None, ignore_backoff=False):
+                 timeout=None, ignore_backoff=False,
+                 try_trailing_slash_on_400=False):
         """ GETs some json from the given host homeserver and path
 
         Args:
@@ -606,6 +665,9 @@ class MatrixFederationHttpClient(object):
                 be retried.
             ignore_backoff (bool): true to ignore the historical backoff data
                 and try the request anyway.
+            try_trailing_slash_on_400 (bool): True if on a 400 M_UNRECOGNIZED
+                response we should try appending a trailing slash to the end of
+                the request. Workaround for #3622 in Synapse <= v0.99.3.
         Returns:
             Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The
             result will be the decoded JSON body.
@@ -631,16 +693,19 @@ class MatrixFederationHttpClient(object):
             query=args,
         )
 
-        response = yield self._send_request(
+        response = yield self._send_request_with_optional_trailing_slash(
             request,
+            try_trailing_slash_on_400,
+            backoff_on_404=False,
+            ignore_backoff=ignore_backoff,
             retry_on_dns_fail=retry_on_dns_fail,
             timeout=timeout,
-            ignore_backoff=ignore_backoff,
         )
 
         body = yield _handle_json_response(
             self.hs.get_reactor(), self.default_timeout, request, response,
         )
+
         defer.returnValue(body)
 
     @defer.inlineCallbacks

tests/handlers/test_typing.py

@@ -192,6 +192,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase):
             json_data_callback=ANY,
             long_retries=True,
             backoff_on_404=True,
+            try_trailing_slash_on_400=True,
         )
 
     def test_started_typing_remote_recv(self):
@@ -269,6 +270,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase):
             json_data_callback=ANY,
             long_retries=True,
             backoff_on_404=True,
+            try_trailing_slash_on_400=True,
         )
 
         self.assertEquals(self.event_source.get_current_key(), 1)

tests/http/test_fedclient.py

@@ -268,6 +268,105 @@ class FederationClientTests(HomeserverTestCase):
 
         self.assertIsInstance(f.value, TimeoutError)
 
+    def test_client_requires_trailing_slashes(self):
+        """
+        If a connection is made to a client but the client rejects it due to
+        requiring a trailing slash. We need to retry the request with a
+        trailing slash. Workaround for Synapse <= v0.99.3, explained in #3622.
+        """
+        d = self.cl.get_json(
+            "testserv:8008", "foo/bar", try_trailing_slash_on_400=True,
+        )
+
+        # Send the request
+        self.pump()
+
+        # there should have been a call to connectTCP
+        clients = self.reactor.tcpClients
+        self.assertEqual(len(clients), 1)
+        (_host, _port, factory, _timeout, _bindAddress) = clients[0]
+
+        # complete the connection and wire it up to a fake transport
+        client = factory.buildProtocol(None)
+        conn = StringTransport()
+        client.makeConnection(conn)
+
+        # that should have made it send the request to the connection
+        self.assertRegex(conn.value(), b"^GET /foo/bar")
+
+        # Clear the original request data before sending a response
+        conn.clear()
+
+        # Send the HTTP response
+        client.dataReceived(
+            b"HTTP/1.1 400 Bad Request\r\n"
+            b"Content-Type: application/json\r\n"
+            b"Content-Length: 59\r\n"
+            b"\r\n"
+            b'{"errcode":"M_UNRECOGNIZED","error":"Unrecognized request"}'
+        )
+
+        # We should get another request with a trailing slash
+        self.assertRegex(conn.value(), b"^GET /foo/bar/")
+
+        # Send a happy response this time
+        client.dataReceived(
+            b"HTTP/1.1 200 OK\r\n"
+            b"Content-Type: application/json\r\n"
+            b"Content-Length: 2\r\n"
+            b"\r\n"
+            b'{}'
+        )
+
+        # We should get a successful response
+        r = self.successResultOf(d)
+        self.assertEqual(r, {})
+
+    def test_client_does_not_retry_on_400_plus(self):
+        """
+        Another test for trailing slashes but now test that we don't retry on
+        trailing slashes on a non-400/M_UNRECOGNIZED response.
+
+        See test_client_requires_trailing_slashes() for context.
+        """
+        d = self.cl.get_json(
+            "testserv:8008", "foo/bar", try_trailing_slash_on_400=True,
+        )
+
+        # Send the request
+        self.pump()
+
+        # there should have been a call to connectTCP
+        clients = self.reactor.tcpClients
+        self.assertEqual(len(clients), 1)
+        (_host, _port, factory, _timeout, _bindAddress) = clients[0]
+
+        # complete the connection and wire it up to a fake transport
+        client = factory.buildProtocol(None)
+        conn = StringTransport()
+        client.makeConnection(conn)
+
+        # that should have made it send the request to the connection
+        self.assertRegex(conn.value(), b"^GET /foo/bar")
+
+        # Clear the original request data before sending a response
+        conn.clear()
+
+        # Send the HTTP response
+        client.dataReceived(
+            b"HTTP/1.1 404 Not Found\r\n"
+            b"Content-Type: application/json\r\n"
+            b"Content-Length: 2\r\n"
+            b"\r\n"
+            b"{}"
+        )
+
+        # We should not get another request
+        self.assertEqual(conn.value(), b"")
+
+        # We should get a 404 failure response
+        self.failureResultOf(d)
+
     def test_client_sends_body(self):
         self.cl.post_json(
             "testserv:8008", "foo/bar", timeout=10000,