Follow redirects when fetching from /download.

Author: clokep

synapse/federation/transport/client.py

@@ -850,7 +850,11 @@ async def download_media_v3(
                 # end up with a routing loop.
                 "allow_remote": "false",
                 "timeout_ms": str(max_timeout_ms),
+                # Matix 1.7 allows for this to redirect to another URL, this should
+                # just be ignored for an old homeserver, so always provide it.
+                "allow_redirect": "true",
             },
+            follow_redirects=True,
         )
 
 

synapse/http/matrixfederationclient.py

@@ -164,8 +164,8 @@ def _create_txn_id(self) -> str:
         _next_id = (_next_id + 1) % (MAXINT - 1)
         return txn_id
 
-    uri: bytes = attr.ib(init=False)
-    """The URI of this request, generated from the above information.
+    uri: bytes = attr.ib()
+    """The URI of this request, usually generated from the above information.
     """
 
     @uri.default
@@ -515,6 +515,7 @@ async def _send_request(
         ignore_backoff: bool = False,
         backoff_on_404: bool = False,
         backoff_on_all_error_codes: bool = False,
+        follow_redirects: bool = False,
     ) -> IResponse:
         """
         Sends a request to the given server.
@@ -557,6 +558,9 @@ async def _send_request(
             backoff_on_404: Back off if we get a 404
             backoff_on_all_error_codes: Back off if we get any error response
 
+            follow_redirects: True to follow the Location header of 307/308 redirect
+                responses. This does not recurse.
+
         Returns:
             Resolves with the HTTP response object on success.
 
@@ -716,6 +720,26 @@ async def _send_request(
                             response.code,
                             response_phrase,
                         )
+                    elif (
+                        response.code in (307, 308)
+                        and follow_redirects
+                        and response.headers.hasHeader("Location")
+                    ):
+                        # The Location header *might* be relative so resolve it.
+                        location = response.headers.getRawHeaders(b"Location")[0]
+                        new_uri = urllib.parse.urljoin(request.uri, location)
+
+                        return await self._send_request(
+                            attr.evolve(request, uri=new_uri),
+                            retry_on_dns_fail,
+                            timeout,
+                            long_retries,
+                            ignore_backoff,
+                            backoff_on_404,
+                            backoff_on_all_error_codes,
+                            # Do not continue following redirects.
+                            follow_redirects=False,
+                        )
                     else:
                         logger.info(
                             "{%s} [%s] Got response headers: %d %s",
@@ -1385,6 +1409,7 @@ async def get_file(
         retry_on_dns_fail: bool = True,
         max_size: Optional[int] = None,
         ignore_backoff: bool = False,
+        follow_redirects: bool = False,
     ) -> Tuple[int, Dict[bytes, List[bytes]]]:
         """GETs a file from a given homeserver
         Args:
@@ -1394,6 +1419,8 @@ async def get_file(
             args: Optional dictionary used to create the query string.
             ignore_backoff: true to ignore the historical backoff data
                 and try the request anyway.
+            follow_redirects: True to follow the Location header of 307/308 redirect
+                responses. This does not recurse.
 
         Returns:
             Resolves with an (int,dict) tuple of
@@ -1414,7 +1441,10 @@ async def get_file(
         )
 
         response = await self._send_request(
-            request, retry_on_dns_fail=retry_on_dns_fail, ignore_backoff=ignore_backoff
+            request,
+            retry_on_dns_fail=retry_on_dns_fail,
+            ignore_backoff=ignore_backoff,
+            follow_redirects=follow_redirects,
         )
 
         headers = dict(response.headers.getAllRawHeaders())

tests/media/test_media_storage.py

@@ -248,6 +248,7 @@ def get_file(
             retry_on_dns_fail: bool = True,
             max_size: Optional[int] = None,
             ignore_backoff: bool = False,
+            follow_redirects: bool = False,
         ) -> "Deferred[Tuple[int, Dict[bytes, List[bytes]]]]":
             """A mock for MatrixFederationHttpClient.get_file."""
 
@@ -325,7 +326,8 @@ def _req(
             self.fetches[0][2], "/_matrix/media/v3/download/" + self.media_id
         )
         self.assertEqual(
-            self.fetches[0][3], {"allow_remote": "false", "timeout_ms": "20000"}
+            self.fetches[0][3],
+            {"allow_remote": "false", "timeout_ms": "20000", "allow_redirect": "true"},
         )
 
         headers = {