From 288f4aeafcf828c017d09abf83a7eb6296c7ae63 Mon Sep 17 00:00:00 2001 From: "feng.tsai" Date: Tue, 8 Sep 2026 10:35:07 +0800 Subject: [PATCH] fix(guardrails): match the downstream remote-url predicate exactly The remote-image refusal tested `startswith(("http://", "https://"))` while BedrockImageProcessor.process_image_async decides with `"http://" in image_url or "https://" in image_url`. A url carrying leading whitespace failed the prefix test, then matched downstream and was fetched -- the fail-closed policy turned into an uncapped server-side download, which is exactly the remote-fetch work this PR defers (async_safe_get still has no size cap on the base). Aligned the two predicates and parametrized the rejection test over leading space, tab and newline, asserting the decoder is never awaited. Reverting the predicate fails the three whitespace cases. Reported by Veria AI on #38175. --- .../guardrail_hooks/bedrock_guardrails.py | 10 +++++- .../test_bedrock_guardrails.py | 34 +++++++++++++------ 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/litellm/proxy/guardrails/guardrail_hooks/bedrock_guardrails.py b/litellm/proxy/guardrails/guardrail_hooks/bedrock_guardrails.py index 635d6b7b640..0c78d626b49 100644 --- a/litellm/proxy/guardrails/guardrail_hooks/bedrock_guardrails.py +++ b/litellm/proxy/guardrails/guardrail_hooks/bedrock_guardrails.py @@ -606,8 +606,16 @@ class BedrockGuardrail(CustomGuardrail, BaseAWSLLM): same bytes to the model), so the operator gets "not supported" instead of the decoder's "could not be read". Anything else that is not a data URI is an unrecognized payload and falls through to the decoder, which rejects it. + + The test is a substring, not a prefix, deliberately: it has to reject + everything `BedrockImageProcessor.process_image_async` would treat as + remote, and that check is `"http://" in image_url or "https://" in + image_url`. A prefix test reads more naturally but leaves a hole -- a url + carrying leading whitespace fails it, then matches downstream and is + fetched, so the fail-closed policy here would be bypassed into an + uncapped server-side download. Keep the two predicates identical. """ - if image_url.startswith(("http://", "https://")): + if "http://" in image_url or "https://" in image_url: self._handle_unscannable_image(reason="remote image URLs are not supported") try: diff --git a/tests/test_litellm/proxy/guardrails/guardrail_hooks/test_bedrock_guardrails.py b/tests/test_litellm/proxy/guardrails/guardrail_hooks/test_bedrock_guardrails.py index 4b75f52f282..614decd3d36 100644 --- a/tests/test_litellm/proxy/guardrails/guardrail_hooks/test_bedrock_guardrails.py +++ b/tests/test_litellm/proxy/guardrails/guardrail_hooks/test_bedrock_guardrails.py @@ -5589,24 +5589,38 @@ class TestBedrockGuardrailImageInput: assert "could not be read" in str(exc_info.value.detail) or "not a png/jpeg" in str(exc_info.value.detail) + @pytest.mark.parametrize( + "url", + [ + pytest.param("https://example.com/pic.png", id="plain https url"), + pytest.param("http://example.com/pic.png", id="plain http url"), + # The rejection used to be a prefix test, which these slip past while + # `process_image_async` still treats them as remote and fetches them -- + # an uncapped server-side download straight through the fail-closed path. + pytest.param(" https://example.com/pic.png", id="leading space"), + pytest.param("\thttps://example.com/pic.png", id="leading tab"), + pytest.param("\nhttps://example.com/pic.png", id="leading newline"), + ], + ) @pytest.mark.asyncio - async def test_a_remote_image_url_is_rejected_without_being_fetched(self): + async def test_a_remote_image_url_is_rejected_without_being_fetched(self, url): """A remote url is named as its own rejection rather than left to the decoder. Fetching one safely (size cap, SSRF/redirect validation) is separate work; this PR only scans inline images, so a url has to fail closed rather than be ignored - or silently forwarded to the model unscanned. + or silently forwarded to the model unscanned. The refusal has to catch every + shape the decoder would fetch, not just the well-formed ones. """ - messages = [ - { - "role": "user", - "content": [{"type": "image_url", "image_url": {"url": "https://example.com/pic.png"}}], - } - ] + messages = [{"role": "user", "content": [{"type": "image_url", "image_url": {"url": url}}]}] - with pytest.raises(HTTPException) as exc_info: - await self._guardrail().convert_to_bedrock_format(source="INPUT", messages=messages) + with patch( # test-quality-ok: the fetch is the thing under assertion -- it must never be reached + "litellm.proxy.guardrails.guardrail_hooks.bedrock_guardrails.BedrockImageProcessor.process_image_async", + new_callable=AsyncMock, + ) as decode: + with pytest.raises(HTTPException) as exc_info: + await self._guardrail().convert_to_bedrock_format(source="INPUT", messages=messages) + decode.assert_not_awaited() assert "remote image URLs are not supported" in str(exc_info.value.detail) def test_file_backed_part_counting_skips_non_mapping_entries(self):