fix(s3_v2): use prepared URL for SigV4-signed S3 requests by nehaaprasad · Pull Request #25074 · BerriAI/litellm

nehaaprasad · 2026-04-03T10:51:11Z

fixes #25019

Summary

SigV4 was computed on requests’ prepared URL (%20 for spaces) while httpx used the raw URL (literal spaces), causing signature mismatch and 403 on PUT/GET.
Use prepped.url for httpx so the request matches the signed URL.

Changes

litellm/integrations/s3_v2.py: async upload, sync upload, and download now call httpx with prepped.url.
tests/test_litellm/integrations/test_s3_v2.py: test for object keys with spaces (e.g. team name in prefix).

Type

Bug fix

vercel · 2026-04-03T10:51:16Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
litellm	Ready	Preview, Comment	Apr 3, 2026 10:52am

codspeed-hq · 2026-04-03T10:53:42Z

Merging this PR will not alter performance

✅ 16 untouched benchmarks

_{Comparing nehaaprasad:fix/s3-team-name (57b880f) with main (d4a3a5e)}

greptile-apps · 2026-04-03T10:53:48Z

Greptile Summary

This PR fixes a SigV4 signature mismatch on S3 PUT/GET requests when the object key contains characters that need percent-encoding (e.g. spaces in team-name prefixes). Previously, SigV4 was computed over the percent-encoded prepped.url while httpx was handed the raw URL with literal spaces, causing a 403. The fix is a one-liner in each of the three affected call sites — passing prepped.url (already computed for signing) to httpx instead of the raw url string.

litellm/integrations/s3_v2.py — Correct and minimal: three call sites (async_upload_data_to_s3, upload_data_to_s3, _download_object_from_s3) now consistently use prepped.url.
tests/test_litellm/integrations/test_s3_v2.py — New mock test validates the fix for the async upload path; no real network calls are made. Minor style issues: three imports are placed inline inside the test method instead of at module level (violates CLAUDE.md guideline), a blank line is missing before the next module-level function, and the sync upload / download paths are not covered by equivalent tests.

Confidence Score: 5/5

Safe to merge — the bug fix is correct and all remaining findings are minor style issues.

The core fix is correct and consistent across all three affected call sites. No logic, security, or data-integrity issues were found. All remaining comments (inline imports, missing blank line, incomplete test coverage of sync/download paths) are P2 style and quality suggestions that do not block correctness.

No files require special attention; tests/test_litellm/integrations/test_s3_v2.py has minor style issues with inline imports.

Important Files Changed

Filename	Overview
litellm/integrations/s3_v2.py	Replaces raw `url` with `prepped.url` (percent-encoded) in three HTTP call sites (async PUT, sync PUT, async GET), ensuring the URL sent to httpx matches the URL that was SigV4-signed — a correct and minimal fix.
tests/test_litellm/integrations/test_s3_v2.py	Adds a mock-only unit test verifying the async upload URL-encodes spaces; inline imports and a missing blank line are minor style issues, and sync/download paths lack analogous coverage.

Sequence Diagram

sequenceDiagram
    participant S3Logger
    participant Prep as requests.prepare()
    participant SigV4
    participant HTTPX

    S3Logger->>Prep: prepare(method, raw_url)
    Prep-->>S3Logger: prepped.url (percent-encoded)
    S3Logger->>SigV4: sign(prepped.url)
    SigV4-->>S3Logger: signed_headers
    S3Logger->>HTTPX: request(prepped.url, signed_headers)
    Note over HTTPX: URL matches signed URL

_{Reviews (1): Last reviewed commit: "fix(s3_v2): use prepared URL for SigV4-s..." | Re-trigger Greptile}

greptile-apps · 2026-04-03T10:53:52Z

+        import requests
+        from unittest.mock import AsyncMock
+
+        from litellm.types.integrations.s3_v2 import s3BatchLoggingElement


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!}

greptile-apps · 2026-04-03T10:53:53Z

+
 @pytest.mark.asyncio


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

greptile-apps · 2026-04-03T10:53:54Z

+    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
+
+        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


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.

codecov · 2026-04-03T11:08:43Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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

57b880f

vercel bot deployed to Preview April 3, 2026 10:52 View deployment

greptile-apps bot reviewed Apr 3, 2026

View reviewed changes

krrish-berri-2 changed the base branch from main to litellm_oss_staging_04_04_2026 April 5, 2026 01:24

krrish-berri-2 merged commit d6351a3 into BerriAI:litellm_oss_staging_04_04_2026 Apr 5, 2026
59 of 62 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix(s3_v2): use prepared URL for SigV4-signed S3 requests#25074
krrish-berri-2 merged 1 commit intoBerriAI:litellm_oss_staging_04_04_2026from
nehaaprasad:fix/s3-team-name

nehaaprasad commented Apr 3, 2026

Uh oh!

vercel bot commented Apr 3, 2026 •

edited

Loading

Uh oh!

codspeed-hq bot commented Apr 3, 2026

Uh oh!

greptile-apps bot commented Apr 3, 2026

Important Files Changed

Uh oh!

greptile-apps bot Apr 3, 2026

Uh oh!

greptile-apps bot Apr 3, 2026

Uh oh!

greptile-apps bot Apr 3, 2026

Uh oh!

codecov bot commented Apr 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        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

Uh oh!

Conversation

nehaaprasad commented Apr 3, 2026

Summary

Changes

Type

Uh oh!

vercel bot commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codspeed-hq bot commented Apr 3, 2026

Merging this PR will not alter performance

Uh oh!

greptile-apps bot commented Apr 3, 2026

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 3, 2026

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vercel bot commented Apr 3, 2026 •

edited

Loading