From 8f6de53c92a8d1bb357d9a7a24b11abe1e06eeb0 Mon Sep 17 00:00:00 2001 From: "devin-ai-integration[bot]" <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 24 Aug 2026 18:16:17 -0700 Subject: [PATCH] fix(s3_v2): percent-encode object keys once so signed and sent URLs match (#38005) Co-authored-by: yucheng Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- litellm/integrations/s3_v2.py | 99 ++++----------- tests/test_litellm/integrations/test_s3_v2.py | 116 +++++++++++++++++- 2 files changed, 141 insertions(+), 74 deletions(-) diff --git a/litellm/integrations/s3_v2.py b/litellm/integrations/s3_v2.py index d52bcda525f..10b4c0dd433 100644 --- a/litellm/integrations/s3_v2.py +++ b/litellm/integrations/s3_v2.py @@ -11,6 +11,7 @@ import time from collections.abc import Mapping from datetime import datetime from typing import Final, cast +from urllib.parse import quote import litellm from litellm._logging import print_verbose, verbose_logger @@ -206,6 +207,23 @@ class S3Logger(CustomBatchLogger, BaseAWSLLM): params.get("s3_sse_kms_key_id") or s3_sse_kms_key_id, ) + def _build_object_url(self, s3_object_key: str) -> str: + """ + Build the exact URL that is both signed and sent, with the key percent-encoded once. + + S3SigV4Auth signs the path verbatim while S3 canonicalizes the received path with reserved + characters encoded, so an unencoded `=`, `+`, `&`, `#`, `?`, `%` or space in the key makes + the two signatures disagree (403 SignatureDoesNotMatch). + """ + encoded_key: Final = quote(s3_object_key, safe="/") + if self.s3_endpoint_url and self.s3_bucket_name: + if self.s3_use_virtual_hosted_style: + endpoint_host: Final = self.s3_endpoint_url.replace("https://", "").replace("http://", "") + protocol: Final = "https://" if self.s3_endpoint_url.startswith("https://") else "http://" + return f"{protocol}{self.s3_bucket_name}.{endpoint_host}/{encoded_key}" + return f"{self.s3_endpoint_url}/{self.s3_bucket_name}/{encoded_key}" + return f"https://{self.s3_bucket_name}.s3.{self.s3_region_name}.amazonaws.com/{encoded_key}" + def _sse_headers(self) -> Mapping[str, str]: candidates: Final = { "x-amz-server-side-encryption": self.s3_server_side_encryption, @@ -292,7 +310,6 @@ class S3Logger(CustomBatchLogger, BaseAWSLLM): import base64 import hashlib - import requests from botocore.auth import S3SigV4Auth from botocore.awsrequest import AWSRequest except ImportError: @@ -316,18 +333,7 @@ class S3Logger(CustomBatchLogger, BaseAWSLLM): verbose_logger.debug("s3_v2 logger - uploading data to s3 - %s", batch_logging_element.s3_object_key) verbose_logger.debug("s3_v2 logger - s3_verify setting: %s", self.s3_verify) - # Prepare the URL - url = f"https://{self.s3_bucket_name}.s3.{self.s3_region_name}.amazonaws.com/{batch_logging_element.s3_object_key}" - - if self.s3_endpoint_url and self.s3_bucket_name: - if self.s3_use_virtual_hosted_style: - # Virtual-hosted-style: bucket.endpoint/key - endpoint_host: Final = self.s3_endpoint_url.replace("https://", "").replace("http://", "") - protocol: Final = "https://" if self.s3_endpoint_url.startswith("https://") else "http://" - url = f"{protocol}{self.s3_bucket_name}.{endpoint_host}/{batch_logging_element.s3_object_key}" - else: - # Path-style: endpoint/bucket/key - url = self.s3_endpoint_url + "/" + self.s3_bucket_name + "/" + batch_logging_element.s3_object_key + url: Final = self._build_object_url(batch_logging_element.s3_object_key) # Convert JSON to string json_string: Final = safe_dumps(batch_logging_element.payload) @@ -348,29 +354,19 @@ class S3Logger(CustomBatchLogger, BaseAWSLLM): "Cache-Control": "private, immutable, max-age=31536000, s-maxage=0", **self._sse_headers(), } - req: Final = requests.Request("PUT", url, data=json_string, headers=headers) - prepped: Final = req.prepare() # Sign the request - aws_request: Final = AWSRequest( - method=prepped.method, - url=prepped.url, - data=prepped.body, - headers=prepped.headers, - ) + aws_request: Final = AWSRequest(method="PUT", url=url, data=json_string, headers=headers) aws_region_name: Final = self.get_aws_region_name_for_non_llm_api_calls(aws_region_name=self.s3_region_name) S3SigV4Auth(credentials, "s3", aws_region_name).add_auth(aws_request) # Prepare the signed headers signed_headers: Final = dict(aws_request.headers.items()) - # Use prepared URL so path segments match SigV4 canonical request (e.g. %20 for spaces). - request_url: Final = prepped.url or url - # Make the request with retry for transient S3 errors (500/503) max_retries: Final = 3 for attempt in range(max_retries): - response = await self.async_httpx_client.put(request_url, data=json_string, headers=signed_headers) + response = await self.async_httpx_client.put(url, data=json_string, headers=signed_headers) if response.status_code in (500, 503) and attempt < max_retries - 1: wait_time = 2**attempt # 1s, 2s verbose_logger.warning( @@ -478,7 +474,6 @@ class S3Logger(CustomBatchLogger, BaseAWSLLM): import base64 import hashlib - import requests from botocore.auth import S3SigV4Auth from botocore.awsrequest import AWSRequest from botocore.credentials import Credentials @@ -493,18 +488,7 @@ class S3Logger(CustomBatchLogger, BaseAWSLLM): aws_region_name=self.s3_region_name, ) - # Prepare the URL - url = f"https://{self.s3_bucket_name}.s3.{self.s3_region_name}.amazonaws.com/{batch_logging_element.s3_object_key}" - - if self.s3_endpoint_url and self.s3_bucket_name: - if self.s3_use_virtual_hosted_style: - # Virtual-hosted-style: bucket.endpoint/key - endpoint_host: Final = self.s3_endpoint_url.replace("https://", "").replace("http://", "") - protocol: Final = "https://" if self.s3_endpoint_url.startswith("https://") else "http://" - url = f"{protocol}{self.s3_bucket_name}.{endpoint_host}/{batch_logging_element.s3_object_key}" - else: - # Path-style: endpoint/bucket/key - url = self.s3_endpoint_url + "/" + self.s3_bucket_name + "/" + batch_logging_element.s3_object_key + url: Final = self._build_object_url(batch_logging_element.s3_object_key) # Convert JSON to string json_string: Final = safe_dumps(batch_logging_element.payload) @@ -525,32 +509,22 @@ class S3Logger(CustomBatchLogger, BaseAWSLLM): "Cache-Control": "private, immutable, max-age=31536000, s-maxage=0", **self._sse_headers(), } - req: Final = requests.Request("PUT", url, data=json_string, headers=headers) - prepped: Final = req.prepare() # Sign the request - aws_request: Final = AWSRequest( - method=prepped.method, - url=prepped.url, - data=prepped.body, - headers=prepped.headers, - ) + aws_request: Final = AWSRequest(method="PUT", url=url, data=json_string, headers=headers) aws_region_name: Final = self.get_aws_region_name_for_non_llm_api_calls(aws_region_name=self.s3_region_name) S3SigV4Auth(credentials, "s3", aws_region_name).add_auth(aws_request) # Prepare the signed headers signed_headers: Final = dict(aws_request.headers.items()) - # Use prepared URL so path segments match SigV4 canonical request (e.g. %20 for spaces). - request_url: Final = prepped.url or url - httpx_client: Final = _get_httpx_client( params=({"ssl_verify": self.s3_verify} if self.s3_verify is not None else None) ) # Make the request with retry for transient S3 errors (500/503) max_retries: Final = 3 for attempt in range(max_retries): - response = httpx_client.put(request_url, data=json_string, headers=signed_headers) + response = httpx_client.put(url, data=json_string, headers=signed_headers) if response.status_code in (500, 503) and attempt < max_retries - 1: wait_time = 2**attempt # 1s, 2s verbose_logger.warning( @@ -582,7 +556,6 @@ class S3Logger(CustomBatchLogger, BaseAWSLLM): try: import hashlib - import requests from botocore.auth import S3SigV4Auth from botocore.awsrequest import AWSRequest except ImportError: @@ -607,18 +580,7 @@ class S3Logger(CustomBatchLogger, BaseAWSLLM): verbose_logger.debug("s3_v2 logger - downloading data from s3 - %s", s3_object_key) - # Prepare the URL - url = f"https://{self.s3_bucket_name}.s3.{self.s3_region_name}.amazonaws.com/{s3_object_key}" - - if self.s3_endpoint_url and self.s3_bucket_name: - if self.s3_use_virtual_hosted_style: - # Virtual-hosted-style: bucket.endpoint/key - endpoint_host: Final = self.s3_endpoint_url.replace("https://", "").replace("http://", "") - protocol: Final = "https://" if self.s3_endpoint_url.startswith("https://") else "http://" - url = f"{protocol}{self.s3_bucket_name}.{endpoint_host}/{s3_object_key}" - else: - # Path-style: endpoint/bucket/key - url = self.s3_endpoint_url + "/" + self.s3_bucket_name + "/" + s3_object_key + url: Final = self._build_object_url(s3_object_key) # Prepare the request for GET operation # For GET requests, we need x-amz-content-sha256 with hash of empty string @@ -626,22 +588,15 @@ class S3Logger(CustomBatchLogger, BaseAWSLLM): headers: Final = { "x-amz-content-sha256": empty_string_hash, } - req: Final = requests.Request("GET", url, headers=headers) - prepped: Final = req.prepare() # Sign the request - aws_request: Final = AWSRequest( - method=prepped.method, - url=prepped.url, - headers=prepped.headers, - ) + aws_request: Final = AWSRequest(method="GET", url=url, headers=headers) S3SigV4Auth(credentials, "s3", self.s3_region_name).add_auth(aws_request) # Prepare the signed headers signed_headers: Final = dict(aws_request.headers.items()) - request_url: Final = prepped.url or url - response: Final = await self.async_httpx_client.get(request_url, headers=signed_headers) + response: Final = await self.async_httpx_client.get(url, headers=signed_headers) if response.status_code != 200: verbose_logger.exception("S3 object not found, saw response=", response.text) diff --git a/tests/test_litellm/integrations/test_s3_v2.py b/tests/test_litellm/integrations/test_s3_v2.py index 933e41d17a0..0f307450b50 100644 --- a/tests/test_litellm/integrations/test_s3_v2.py +++ b/tests/test_litellm/integrations/test_s3_v2.py @@ -1647,15 +1647,27 @@ def _signature_for(signer_cls, url: str, method: str, body: bytes | None, header return signer.signature(signer.string_to_sign(request, canonical_request), request) +def _as_s3_canonicalizes(url: str) -> str: + """ + The path S3 rebuilds from the wire path: percent-encode everything outside the unreserved + set, without normalizing or double-encoding. `=` becomes `%3D`, `%20` stays `%20`. + """ + from urllib.parse import quote, unquote, urlsplit, urlunsplit + + split = urlsplit(url) + return urlunsplit(split._replace(path=quote(unquote(split.path), safe="/~"))) + + def _assert_signed_for_s3_canonicalization(url: str, method: str, body: bytes | None, headers: dict[str, str]) -> None: """ S3 rebuilds the canonical request from the wire path with single percent-encoding, which botocore models as S3SigV4Auth; plain SigV4Auth double-encodes it (%2520 for a space) and S3 - answers 403 SignatureDoesNotMatch. Assert we signed the path the way S3 reads it. + answers 403 SignatureDoesNotMatch. Assert we sent an already-encoded path and signed it the + way S3 reads it. """ from botocore.auth import S3SigV4Auth, SigV4Auth - assert "%20" in url + assert url == _as_s3_canonicalizes(url) sent_signature = headers["Authorization"].split("Signature=")[1].strip() assert sent_signature == _signature_for(S3SigV4Auth, url, method, body, headers) assert sent_signature != _signature_for(SigV4Auth, url, method, body, headers) @@ -1744,3 +1756,103 @@ async def test_download_signs_object_key_with_space_the_way_s3_does(): body=None, headers=call.kwargs["headers"], ) + +_RESERVED_CHAR_KEYS = ( + "2026-08-21/time-05-29-36_resp_bGl0ZWxsbTpjdXN0b20=.json", + "session=logs/2026-08-21/time-05-29-36_abc.json", + "a+b/2026-08-21/time-05-29-36_abc.json", + "a&b/2026-08-21/time-05-29-36_abc.json", + "a#b/2026-08-21/time-05-29-36_abc.json", + "a?b/2026-08-21/time-05-29-36_abc.json", + "a%b/2026-08-21/time-05-29-36_abc.json", + _KEY_WITH_SPACE, +) + + +def _element_for(s3_object_key: str): + from litellm.types.integrations.s3_v2 import s3BatchLoggingElement + + return s3BatchLoggingElement( + s3_object_key=s3_object_key, + payload={"test": "sigv4"}, + s3_object_download_filename="log.json", + ) + + +def _expected_wire_url(s3_object_key: str) -> str: + """The URL boto3 itself would put on the wire for this key.""" + from urllib.parse import quote + + return f"https://logs-bucket.s3.us-east-1.amazonaws.com/{quote(s3_object_key, safe='/')}" + + +@pytest.mark.parametrize("s3_object_key", _RESERVED_CHAR_KEYS) +@pytest.mark.asyncio +async def test_async_upload_percent_encodes_reserved_characters_in_object_key(s3_object_key): + from unittest.mock import AsyncMock, MagicMock + + logger = _logger_for_signing() + response = MagicMock() + response.status_code = 200 + response.raise_for_status = MagicMock() + logger.async_httpx_client = AsyncMock() + logger.async_httpx_client.put.return_value = response + + await logger.async_upload_data_to_s3(_element_for(s3_object_key)) + + call = logger.async_httpx_client.put.call_args + assert call[0][0] == _expected_wire_url(s3_object_key) + _assert_signed_for_s3_canonicalization( + url=call[0][0], + method="PUT", + body=call.kwargs["data"].encode("utf-8"), + headers=call.kwargs["headers"], + ) + + +@pytest.mark.parametrize("s3_object_key", _RESERVED_CHAR_KEYS) +def test_sync_upload_percent_encodes_reserved_characters_in_object_key(s3_object_key): + from unittest.mock import MagicMock + + logger = _logger_for_signing() + response = MagicMock() + response.status_code = 200 + response.raise_for_status = MagicMock() + mock_sync_client = MagicMock() + mock_sync_client.put.return_value = response + + with patch("litellm.integrations.s3_v2._get_httpx_client", return_value=mock_sync_client): + logger.upload_data_to_s3(_element_for(s3_object_key)) + + call = mock_sync_client.put.call_args + assert call[0][0] == _expected_wire_url(s3_object_key) + _assert_signed_for_s3_canonicalization( + url=call[0][0], + method="PUT", + body=call.kwargs["data"].encode("utf-8"), + headers=call.kwargs["headers"], + ) + + +@pytest.mark.parametrize("s3_object_key", _RESERVED_CHAR_KEYS) +@pytest.mark.asyncio +async def test_download_percent_encodes_reserved_characters_in_object_key(s3_object_key): + from unittest.mock import AsyncMock, MagicMock + + logger = _logger_for_signing() + response = MagicMock() + response.status_code = 200 + response.json = MagicMock(return_value={"downloaded": "data"}) + logger.async_httpx_client = AsyncMock() + logger.async_httpx_client.get.return_value = response + + assert await logger._download_object_from_s3(s3_object_key) == {"downloaded": "data"} + + call = logger.async_httpx_client.get.call_args + assert call[0][0] == _expected_wire_url(s3_object_key) + _assert_signed_for_s3_canonicalization( + url=call[0][0], + method="GET", + body=None, + headers=call.kwargs["headers"], + )