fix: don't send authorization headers with download links

foxglove/client/api.py

@@ -156,19 +156,39 @@ def json_or_raise(response: requests.Response):
     return json
 
 
+def _download_response_with_progress(
+    response: requests.Response,
+    callback: Optional[ProgressCallback] = None,
+):
+    try:
+        response.raise_for_status()
+        data = BytesIO()
+        for chunk in response.iter_content(chunk_size=32 * 1024):
+            data.write(chunk)
+            if callback:
+                callback(progress=data.tell())
+        return data.getvalue()
+    finally:
+        response.close()
+
+
+def _iter_decoded_messages(response: requests.Response, decoder_factories):
+    try:
+        reader = make_reader(response.raw, decoder_factories=decoder_factories)
+        # messages from Foxglove are already in log-time order.
+        # specifying log_time_order=false allows us to skip a sort() in the MCAP library
+        # after all messages are loaded.
+        yield from reader.iter_decoded_messages(log_time_order=False)
+    finally:
+        response.close()
+
+
 def _download_stream_with_progress(
     url: str,
-    session: requests.Session,
     callback: Optional[ProgressCallback] = None,
 ):
-    response = session.get(url, stream=True)
-    response.raise_for_status()
-    data = BytesIO()
-    for chunk in response.iter_content(chunk_size=32 * 1024):
-        data.write(chunk)
-        if callback:
-            callback(progress=data.tell())
-    return data.getvalue()
+    response = requests.get(url, stream=True)
+    return _download_response_with_progress(response, callback=callback)
 
 
 class Client:
@@ -437,16 +457,16 @@ def iter_messages(
             topics=topics,
             project_id=project_id,
         )
-        response = self.__session.get(stream_link, stream=True)
-        response.raise_for_status()
+        response = requests.get(stream_link, stream=True)
+        try:
+            response.raise_for_status()
+        except Exception:
+            response.close()
+            raise
         if decoder_factories is None:
             # We deep-copy here as these factories might be mutated
             decoder_factories = copy.deepcopy(DEFAULT_DECODER_FACTORIES)
-        reader = make_reader(response.raw, decoder_factories=decoder_factories)
-        # messages from Foxglove are already in log-time order.
-        # specifying log_time_order=false allows us to skip a sort() in the MCAP library
-        # after all messages are loaded.
-        return reader.iter_decoded_messages(log_time_order=False)
+        return _iter_decoded_messages(response, decoder_factories)
 
     def download_recording_data(
         self,
@@ -482,9 +502,7 @@ def download_recording_data(
 
         json = json_or_raise(link_response)
 
-        return _download_stream_with_progress(
-            json["link"], self.__session, callback=callback
-        )
+        return _download_stream_with_progress(json["link"], callback=callback)
 
     def _make_stream_link(
         self,
@@ -582,7 +600,6 @@ def download_data(
                 compression_format=compression_format,
                 project_id=project_id,
             ),
-            self.__session,
             callback=callback,
         )
 
@@ -1020,11 +1037,21 @@ def download_attachment(
         :param callback: a callback to track download progress
         :returns: The downloaded attachment bytes.
         """
-        return _download_stream_with_progress(
+        response = self.__session.get(
             self.__url__(f"/v1/recording-attachments/{id}/download"),
-            self.__session,
-            callback=callback,
+            stream=True,
+            allow_redirects=False,
         )
+        if response.is_redirect:
+            location = response.headers.get("Location")
+            response.close()
+            if location is None:
+                raise requests.exceptions.HTTPError(
+                    "Redirect response missing Location header",
+                    response=response,
+                )
+            return _download_stream_with_progress(location, callback=callback)
+        return _download_response_with_progress(response, callback=callback)
 
     def get_topics(
         self,
@@ -1147,7 +1174,7 @@ def upload_data(
 
         link = json["link"]
         buffer = ProgressBufferReader(data, callback=callback)
-        upload_request = self.__session.put(
+        upload_request = requests.put(
             link,
             data=buffer,
             headers={"Content-Type": "application/octet-stream"},

tests/test_attachments.py

@@ -76,3 +76,27 @@ def test_download_attachment():
     client = Client("test")
     response_data = client.download_attachment(id=id)
     assert data == response_data
+
+
+@responses.activate
+def test_download_attachment_signed_url_uses_unauthenticated_request():
+    id = "abcde"
+    download_link = fake.url()
+    data = fake.binary(4096)
+    responses.add(
+        responses.GET,
+        api_url(f"/v1/recording-attachments/{id}/download"),
+        status=302,
+        headers={"Location": download_link},
+    )
+    responses.add(responses.GET, download_link, body=data)
+    client = Client("test")
+
+    response_data = client.download_attachment(id=id)
+
+    assert data == response_data
+    api_request_headers = responses.calls[0].request.headers
+    signed_request_headers = responses.calls[1].request.headers
+    assert api_request_headers.get("Authorization") == "Bearer test"
+    assert "Authorization" not in signed_request_headers
+    assert signed_request_headers.get("Content-Type") is None

tests/test_data.py

@@ -32,6 +32,30 @@ def test_download():
     assert data == response_data
 
 
+@responses.activate
+def test_download_signed_url_uses_unauthenticated_request():
+    download_link = fake.url()
+    responses.add(
+        responses.POST,
+        api_url("/v1/data/stream"),
+        json={
+            "link": download_link,
+        },
+    )
+    responses.add(responses.GET, download_link, body=fake.binary(4096))
+    client = Client("test")
+
+    client.download_data(
+        device_id="test_id",
+        start=datetime.now(),
+        end=datetime.now(),
+    )
+
+    signed_request_headers = responses.calls[1].request.headers
+    assert "Authorization" not in signed_request_headers
+    assert signed_request_headers.get("Content-Type") is None
+
+
 @responses.activate
 def test_download_with_device_name():
     download_link = fake.url()
@@ -232,6 +256,30 @@ def test_upload():
     assert upload_response["link"] == upload_link
 
 
+@responses.activate
+def test_upload_signed_url_uses_unauthenticated_request():
+    upload_link = fake.url()
+    responses.add(
+        responses.POST,
+        api_url("/v1/data/upload"),
+        json={
+            "link": upload_link,
+        },
+    )
+    responses.add(responses.PUT, upload_link)
+    client = Client("test")
+
+    client.upload_data(
+        device_id="test_device_id",
+        filename="test_file.mcap",
+        data=fake.binary(4096),
+    )
+
+    signed_request_headers = responses.calls[1].request.headers
+    assert "Authorization" not in signed_request_headers
+    assert signed_request_headers.get("Content-Type") == "application/octet-stream"
+
+
 @responses.activate
 def test_upload_deviceless():
     upload_link = fake.url()

tests/test_stream_messages.py

@@ -19,10 +19,13 @@ def __init__(self):
         def raise_for_status(self):
             return None
 
+        def close(self):
+            return None
+
     return Resp()
 
 
-@patch("requests.Session.get", side_effect=get_generated_data)
+@patch("requests.get", side_effect=get_generated_data)
 def test_boot(arg):
     client = Client("test")
     client._make_stream_link = MagicMock(return_value="the_link")