[SYNPY-1525] Verify expiration param in url (#1139)

* Verify expiration param in url

Author: BryanFauble

synapseclient/core/download/download_async.py

@@ -214,6 +214,7 @@ def _generate_chunk_ranges(
 def _pre_signed_url_expiration_time(url: str) -> datetime:
     """
     Returns time at which a presigned url will expire
+    for an AWS S3 pre-signed url
 
     Arguments:
         url: A pre-signed download url from AWS

synapseclient/core/download/download_functions.py

@@ -743,7 +743,10 @@ def _ftp_report_hook(
             )
 
             try:
-                url_has_expiration = "Expires" in urllib_urlparse.urlparse(url).query
+                url_query = urllib_urlparse.urlparse(url).query
+                url_has_expiration = (
+                    "Expires" in url_query and "X-Amz-Expires" in url_query
+                )
                 url_is_expired = False
                 if url_has_expiration:
                     url_is_expired = datetime.datetime.now(
@@ -773,8 +776,9 @@ def _ftp_report_hook(
                 exceptions._raise_for_status(response, verbose=client.debug)
             except SynapseHTTPError as err:
                 if err.response.status_code == 403:
+                    url_query = urllib_urlparse.urlparse(url).query
                     url_has_expiration = (
-                        "Expires" in urllib_urlparse.urlparse(url).query
+                        "Expires" in url_query and "X-Amz-Expires" in url_query
                     )
                     url_is_expired = False
                     if url_has_expiration:

tests/unit/synapseclient/core/download/unit_test_download_async.py

@@ -133,6 +133,7 @@ async def test_pre_signed_url_expiration_time(self) -> None:
             "&X-Amz-Expires=86400"
             "&X-Amz-SignedHeaders=host"
             "&X-Amz-Signature=signature-value"
+            "&Expires=1715000000"
         )
 
         expected = (

tests/unit/synapseclient/core/unit_test_download.py

@@ -834,8 +834,16 @@ async def test_download_md5_mismatch_local_file(self) -> None:
             assert not mocked_remove.called
 
     async def test_download_expired_url(self, syn: Synapse) -> None:
-        url = "http://www.ayy.lmao/filerino.txt?Expires=0"
-        new_url = "http://www.ayy.lmao/new_url.txt?Expires=1715000000"
+        url = (
+            "http://www.ayy.lmao/filerino.txt?Expires=0"
+            "X-Amz-Date=20240509T180000Z"
+            "&X-Amz-Expires=1000"
+        )
+        new_url = (
+            "http://www.ayy.lmao/new_url.txt?Expires=1715000000"
+            "X-Amz-Date=20240509T180000Z"
+            "&X-Amz-Expires=1000"
+        )
         contents = "\n".join(str(i) for i in range(1000))
         temp_destination = os.path.normpath(
             os.path.expanduser("~/fake/path/filerino.txt.temp")
@@ -899,6 +907,70 @@ async def test_download_expired_url(self, syn: Synapse) -> None:
                 auth=None,
             )
 
+    async def test_download_no_aws_expiration(self, syn: Synapse) -> None:
+        url = "http://www.ayy.lmao/filerino.txt?Expires=0"
+        contents = "\n".join(str(i) for i in range(1000))
+        temp_destination = os.path.normpath(
+            os.path.expanduser("~/fake/path/filerino.txt.temp")
+        )
+        destination = os.path.normpath(os.path.expanduser("~/fake/path/filerino.txt"))
+
+        mock_requests_get = MockRequestGetFunction(
+            [
+                create_mock_response(
+                    url,
+                    "stream",
+                    contents=contents,
+                    buffer_size=1024,
+                    partial_end=len(contents),
+                    status_code=200,
+                ),
+            ]
+        )
+        with patch.object(
+            syn._requests_session, "get", side_effect=mock_requests_get
+        ) as mocked_get, patch(
+            "synapseclient.core.download.download_functions._pre_signed_url_expiration_time",
+            return_value=datetime.datetime(1900, 1, 1, tzinfo=datetime.timezone.utc),
+        ) as mocked_pre_signed_url_expiration_time, patch(
+            "synapseclient.core.download.download_functions.get_file_handle_for_download",
+        ) as mocked_get_file_handle_for_download, patch.object(
+            Synapse, "_generate_headers", side_effect=mock_generate_headers
+        ), patch.object(
+            utils, "temp_download_filename", return_value=temp_destination
+        ), patch(
+            "synapseclient.core.download.download_functions.open",
+            new_callable=mock_open(),
+            create=True,
+        ), patch.object(
+            hashlib, "new"
+        ) as mocked_hashlib_new, patch.object(
+            shutil, "move"
+        ), patch.object(
+            os, "remove"
+        ):
+            mocked_hashlib_new.return_value.hexdigest.return_value = "fake md5 is fake"
+            # WHEN I call download_from_url with an expired url
+            download_from_url(
+                url=url,
+                destination=destination,
+                entity_id=OBJECT_ID,
+                file_handle_associate_type=OBJECT_TYPE,
+                expected_md5="fake md5 is fake",
+            )
+            # I expect the expired url to be identified
+            mocked_pre_signed_url_expiration_time.assert_not_called()
+            # AND I expect the URL to be refreshed
+            mocked_get_file_handle_for_download.assert_not_called()
+            # AND I expect the download to be retried with the new URL
+            mocked_get.assert_called_with(
+                url=url,
+                headers=mock_generate_headers(self),
+                stream=True,
+                allow_redirects=False,
+                auth=None,
+            )
+
     async def test_download_url_no_expiration(self, syn: Synapse) -> None:
         url = "http://www.ayy.lmao/filerino.txt"
         contents = "\n".join(str(i) for i in range(1000))