Skip to content

fix(s3_v2): use prepared URL for SigV4-signed S3 requests #25074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
krrish-berri-2 merged 1 commit into from
Apr 5, 2026
Merged

fix(s3_v2): use prepared URL for SigV4-signed S3 requests #25074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions litellm/integrations/s3_v2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,9 +403,8 @@ async def async_upload_data_to_s3(
# Prepare the signed headers
signed_headers = dict(aws_request.headers.items())

# Make the request
response = await self.async_httpx_client.put(
url, data=json_string, headers=signed_headers
prepped.url, data=json_string, headers=signed_headers
)
response.raise_for_status()
except Exception as e:
Expand Down Expand Up @@ -582,8 +581,9 @@ def upload_data_to_s3(self, batch_logging_element: s3BatchLoggingElement):
if self.s3_verify is not None
else None
)
# Make the request
response = httpx_client.put(url, data=json_string, headers=signed_headers)
response = httpx_client.put(
prepped.url, data=json_string, headers=signed_headers
)
response.raise_for_status()
except Exception as e:
verbose_logger.exception(f"Error uploading to s3: {str(e)}")
Expand Down Expand Up @@ -674,8 +674,9 @@ async def _download_object_from_s3(self, s3_object_key: str) -> Optional[dict]:
# Prepare the signed headers
signed_headers = dict(aws_request.headers.items())

# Make the request
response = await self.async_httpx_client.get(url, headers=signed_headers)
response = await self.async_httpx_client.get(
prepped.url, headers=signed_headers
)

if response.status_code != 200:
verbose_logger.exception(
Expand Down
44 changes: 44 additions & 0 deletions tests/test_litellm/integrations/test_s3_v2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,50 @@ def test_s3_v2_virtual_hosted_style(self, mock_periodic_flush, mock_create_task)

assert result == {"downloaded": "data"}

@patch("asyncio.create_task")
@patch("litellm.integrations.s3_v2.CustomBatchLogger.periodic_flush")
def test_s3_v2_put_url_encodes_spaces_in_object_key(
self, mock_periodic_flush, mock_create_task
):
import requests
from unittest.mock import AsyncMock

from litellm.types.integrations.s3_v2 import s3BatchLoggingElement
Comment on lines +300 to +303

@greptile-apps greptile-apps Bot Apr 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Inline imports should be moved to module level

Per the project's style guide (CLAUDE.md), imports inside methods make dependencies harder to trace and hurt readability. requests, AsyncMock, and s3BatchLoggingElement should be declared at the top of the file alongside the existing imports.

The top of the file already imports MagicMock and patch from unittest.mock; AsyncMock can simply be added to that same import:

Suggested change
import requests
from unittest.mock import AsyncMock
from litellm.types.integrations.s3_v2 import s3BatchLoggingElement
import asyncio
from datetime import datetime
from unittest.mock import AsyncMock, MagicMock, patch
import pytest
import requests
from litellm.integrations.s3_v2 import S3Logger
from litellm.types.integrations.s3_v2 import s3BatchLoggingElement
from litellm.types.utils import StandardLoggingPayload

Then remove the three import lines from inside the test method body.

Context Used: CLAUDE.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


mock_periodic_flush.return_value = None
mock_create_task.return_value = None

mock_response = MagicMock()
mock_response.status_code = 200
mock_response.raise_for_status = MagicMock()

s3_object_key = "My Team/2025-09-14/test-key.json"
test_element = s3BatchLoggingElement(
s3_object_key=s3_object_key,
payload={"test": "data"},
s3_object_download_filename="test-file.json",
)

s3_logger = S3Logger(
s3_bucket_name="test-bucket",
s3_endpoint_url="https://s3.amazonaws.com",
s3_aws_access_key_id="test-key",
s3_aws_secret_access_key="test-secret",
s3_region_name="us-east-1",
)
s3_logger.async_httpx_client = AsyncMock()
s3_logger.async_httpx_client.put.return_value = mock_response

asyncio.run(s3_logger.async_upload_data_to_s3(test_element))

call_args = s3_logger.async_httpx_client.put.call_args
assert call_args is not None
actual_url = call_args[0][0]
raw_url = f"https://s3.amazonaws.com/test-bucket/{s3_object_key}"
expected_url = requests.Request("PUT", raw_url).prepare().url
assert actual_url == expected_url
assert " " not in actual_url
Comment on lines +297 to +337

@greptile-apps greptile-apps Bot Apr 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test covers async upload only — sync upload and download paths not tested

The fix applies prepped.url to three code paths: async_upload_data_to_s3, upload_data_to_s3 (sync), and _download_object_from_s3. Only the async upload path has a corresponding test for the URL-encoding behaviour. Consider adding parallel tests for the sync upload and the download path to ensure parity and guard against future regressions on those paths.


@pytest.mark.asyncio
Comment on lines +338 to 339

@greptile-apps greptile-apps Bot Apr 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing blank line between class and module-level function

PEP 8 requires two blank lines before a top-level function definition. There is currently only one blank line between the end of TestS3V2UnitTests and the @pytest.mark.asyncio decorator.

Suggested change
@pytest.mark.asyncio
@pytest.mark.asyncio

async def test_async_log_event_skips_when_standard_logging_object_missing():
"""
Expand Down
Loading