From 6b608e0d92508dc511b6b4e37f8d1e48971a2e39 Mon Sep 17 00:00:00 2001 From: Mike Guida Date: Thu, 25 Jun 2026 15:51:00 -0600 Subject: [PATCH 1/3] fix: don't send authorization headers with download links --- foxglove/client/api.py | 35 +++++++++++++++---------- tests/test_attachments.py | 24 ++++++++++++++++++ tests/test_data.py | 48 +++++++++++++++++++++++++++++++++++ tests/test_stream_messages.py | 2 +- 4 files changed, 95 insertions(+), 14 deletions(-) diff --git a/foxglove/client/api.py b/foxglove/client/api.py index b64bb0c..b54996d 100644 --- a/foxglove/client/api.py +++ b/foxglove/client/api.py @@ -156,12 +156,10 @@ def json_or_raise(response: requests.Response): return json -def _download_stream_with_progress( - url: str, - session: requests.Session, +def _download_response_with_progress( + response: requests.Response, 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): @@ -171,6 +169,14 @@ def _download_stream_with_progress( return data.getvalue() +def _download_stream_with_progress( + url: str, + callback: Optional[ProgressCallback] = None, +): + response = requests.get(url, stream=True) + return _download_response_with_progress(response, callback=callback) + + class Client: def __init__(self, token: str, host: str = "api.foxglove.dev"): self.__token = token @@ -437,7 +443,7 @@ def iter_messages( topics=topics, project_id=project_id, ) - response = self.__session.get(stream_link, stream=True) + response = requests.get(stream_link, stream=True) response.raise_for_status() if decoder_factories is None: # We deep-copy here as these factories might be mutated @@ -482,9 +488,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 +586,6 @@ def download_data( compression_format=compression_format, project_id=project_id, ), - self.__session, callback=callback, ) @@ -1020,11 +1023,17 @@ 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") + if location is None: + response.raise_for_status() + return _download_stream_with_progress(location, callback=callback) + return _download_response_with_progress(response, callback=callback) def get_topics( self, @@ -1147,7 +1156,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"}, diff --git a/tests/test_attachments.py b/tests/test_attachments.py index f84b334..574a423 100644 --- a/tests/test_attachments.py +++ b/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 diff --git a/tests/test_data.py b/tests/test_data.py index 38814bd..3544ebb 100644 --- a/tests/test_data.py +++ b/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() diff --git a/tests/test_stream_messages.py b/tests/test_stream_messages.py index 0ee82ad..010c7e9 100644 --- a/tests/test_stream_messages.py +++ b/tests/test_stream_messages.py @@ -22,7 +22,7 @@ def raise_for_status(self): 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") From 2afb94e1827b006b3987bf7298109eb26a29cdf8 Mon Sep 17 00:00:00 2001 From: Mike Guida Date: Thu, 25 Jun 2026 16:09:04 -0600 Subject: [PATCH 2/3] raise descriptive error --- foxglove/client/api.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/foxglove/client/api.py b/foxglove/client/api.py index b54996d..6aff391 100644 --- a/foxglove/client/api.py +++ b/foxglove/client/api.py @@ -1031,7 +1031,10 @@ def download_attachment( if response.is_redirect: location = response.headers.get("Location") if location is None: - response.raise_for_status() + 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) From 58e46d4ea6c0c14a3c931fa357ac6579172a74f4 Mon Sep 17 00:00:00 2001 From: Mike Guida Date: Thu, 25 Jun 2026 16:40:09 -0600 Subject: [PATCH 3/3] close all responses --- foxglove/client/api.py | 41 ++++++++++++++++++++++++----------- tests/test_stream_messages.py | 3 +++ 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/foxglove/client/api.py b/foxglove/client/api.py index 6aff391..a176824 100644 --- a/foxglove/client/api.py +++ b/foxglove/client/api.py @@ -160,13 +160,27 @@ def _download_response_with_progress( response: requests.Response, callback: Optional[ProgressCallback] = None, ): - 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() + 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( @@ -444,15 +458,15 @@ def iter_messages( project_id=project_id, ) response = requests.get(stream_link, stream=True) - response.raise_for_status() + 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, @@ -1030,6 +1044,7 @@ def download_attachment( ) if response.is_redirect: location = response.headers.get("Location") + response.close() if location is None: raise requests.exceptions.HTTPError( "Redirect response missing Location header", diff --git a/tests/test_stream_messages.py b/tests/test_stream_messages.py index 010c7e9..3e7361a 100644 --- a/tests/test_stream_messages.py +++ b/tests/test_stream_messages.py @@ -19,6 +19,9 @@ def __init__(self): def raise_for_status(self): return None + def close(self): + return None + return Resp()