mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-08 03:08:45 +00:00
* fix: empty guardrails/policies arrays should not trigger enterprise license check (#20304) The UI sends empty arrays for enterprise-only fields (guardrails, policies, logging) even when the user has not configured these features. The backend `is not None` check treated `[]` as a truthy intent to use the feature, falsely requiring an enterprise license for basic team operations. Backend: Add `and updated_kv[field] != [] and updated_kv[field] != {}` guards in `_update_metadata_fields` so empty collections are skipped. UI: Conditionally omit guardrails, logging, and policies from the payload when empty instead of defaulting to `[]`. Fixes #20304 * fix: allow clearing fields with empty collections while skipping enterprise check Address PR review feedback: 1. Move the empty-collection guard into _update_metadata_field (singular) so that empty lists/dicts skip only the premium license check but still get written into metadata. This lets users intentionally clear a previously-set field (e.g. guardrails: []) without being blocked, while the UI's default empty arrays still don't trigger a false enterprise error. 2. Remove sys.path hack from test file; use standard imports that work with pytest discovery. 3. Add tests verifying that empty collections are moved into metadata (field clearing works) even though they bypass the premium check. Fixes #20304 * fix(proxy): add regression tests for #20441 - ensure <script> tags in LLM messages are not blocked The 403 Forbidden error when sending messages containing `<script>` is caused by external WAF/reverse proxy infrastructure (confirmed by the standard nginx HTML 403 response format), not by LiteLLM's own content filtering. However, these regression tests ensure that: 1. The content filter guardrail's built-in patterns do not match HTML tags 2. Messages containing <script> and other HTML tags pass through the content filter unchanged when no explicit HTML-blocking rules are configured 3. The HTTP request body parser correctly handles JSON payloads containing HTML content without modification These tests guard against accidentally introducing HTML/XSS filtering that would break legitimate LLM API usage (e.g., discussing HTML/JavaScript code). Closes #20441 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
3114eb336e
commit
d0163941b2
5 changed files with 382 additions and 4 deletions
|
|
@ -216,7 +216,14 @@ def _update_metadata_field(updated_kv: dict, field_name: str) -> None:
|
|||
field_name: Name of the metadata field being updated
|
||||
"""
|
||||
if field_name in LiteLLM_ManagementEndpoint_MetadataFields_Premium:
|
||||
_premium_user_check()
|
||||
value = updated_kv.get(field_name)
|
||||
# Skip the premium check for empty collections ([] or {}).
|
||||
# The UI sends these as defaults even when the user hasn't configured
|
||||
# any enterprise features (see issue #20304). However, we still
|
||||
# proceed with the update so that users can intentionally clear a
|
||||
# previously-set field by sending an empty list/dict.
|
||||
if value is not None and value != [] and value != {}:
|
||||
_premium_user_check()
|
||||
|
||||
if field_name in updated_kv and updated_kv[field_name] is not None:
|
||||
# remove field from updated_kv
|
||||
|
|
|
|||
|
|
@ -697,3 +697,67 @@ def test_populate_request_with_path_params_does_not_overwrite_existing_values():
|
|||
assert result["organization_id"] == "org-existing" # Should keep original, not "org-query-param"
|
||||
# Verify other data is preserved
|
||||
assert result["messages"] == [{"role": "user", "content": "Hello"}]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_request_body_with_html_script_tags():
|
||||
"""
|
||||
Test that JSON request bodies containing HTML tags like <script> are
|
||||
parsed correctly without being blocked or modified.
|
||||
|
||||
Regression test for GitHub issue #20441:
|
||||
https://github.com/BerriAI/litellm/issues/20441
|
||||
|
||||
LLM message content frequently contains HTML/code snippets.
|
||||
The HTTP parsing layer must not interfere with such content.
|
||||
"""
|
||||
test_messages = [
|
||||
{
|
||||
"role": "user",
|
||||
"content": "<script>alert('hello')</script>",
|
||||
},
|
||||
{
|
||||
"role": "user",
|
||||
"content": "<script> test </script>",
|
||||
},
|
||||
{
|
||||
"role": "user",
|
||||
"content": "Can you explain what <script> tags do in HTML?",
|
||||
},
|
||||
{
|
||||
"role": "user",
|
||||
"content": "Here is code: <div><script src='app.js'></script></div>",
|
||||
},
|
||||
{
|
||||
"role": "user",
|
||||
"content": "<img onerror='alert(1)' src='x'>",
|
||||
},
|
||||
{
|
||||
"role": "user",
|
||||
"content": "<iframe src='https://example.com'></iframe>",
|
||||
},
|
||||
]
|
||||
|
||||
for msg in test_messages:
|
||||
test_payload = {
|
||||
"model": "gpt-4o",
|
||||
"messages": [
|
||||
{"role": "user", "content": "hi"},
|
||||
{"role": "assistant", "content": "Hello! How can I help?"},
|
||||
msg,
|
||||
],
|
||||
}
|
||||
|
||||
mock_request = MagicMock()
|
||||
mock_request.body = AsyncMock(return_value=orjson.dumps(test_payload))
|
||||
mock_request.headers = {"content-type": "application/json"}
|
||||
mock_request.scope = {}
|
||||
|
||||
result = await _read_request_body(mock_request)
|
||||
|
||||
assert result["model"] == "gpt-4o"
|
||||
assert len(result["messages"]) == 3
|
||||
assert result["messages"][2]["content"] == msg["content"], (
|
||||
f"Message content with HTML was modified during parsing: "
|
||||
f"expected={msg['content']!r}, got={result['messages'][2]['content']!r}"
|
||||
)
|
||||
|
|
|
|||
|
|
@ -920,3 +920,148 @@ class TestContentFilterGuardrail:
|
|||
assert (
|
||||
"matched_text" not in detection
|
||||
), "Sensitive content should not be logged"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_html_tags_in_messages_not_blocked(self):
|
||||
"""
|
||||
Test that HTML tags like <script> in LLM message content are NOT blocked
|
||||
by the content filter guardrail.
|
||||
|
||||
Regression test for GitHub issue #20441:
|
||||
https://github.com/BerriAI/litellm/issues/20441
|
||||
|
||||
LLM message content is not rendered as HTML, so HTML tags should be
|
||||
treated as plain text and should pass through without being blocked.
|
||||
"""
|
||||
# Set up a guardrail with all prebuilt patterns enabled as BLOCK
|
||||
patterns = [
|
||||
ContentFilterPattern(
|
||||
pattern_type="prebuilt",
|
||||
pattern_name="us_ssn",
|
||||
action=ContentFilterAction.BLOCK,
|
||||
),
|
||||
ContentFilterPattern(
|
||||
pattern_type="prebuilt",
|
||||
pattern_name="email",
|
||||
action=ContentFilterAction.BLOCK,
|
||||
),
|
||||
ContentFilterPattern(
|
||||
pattern_type="prebuilt",
|
||||
pattern_name="credit_card",
|
||||
action=ContentFilterAction.BLOCK,
|
||||
),
|
||||
]
|
||||
|
||||
guardrail = ContentFilterGuardrail(
|
||||
guardrail_name="test-html-tags",
|
||||
patterns=patterns,
|
||||
)
|
||||
|
||||
# Messages containing <script> and other HTML tags should NOT be blocked
|
||||
html_messages = [
|
||||
"<script>alert('hello')</script>",
|
||||
"<script> test </script>",
|
||||
"Can you explain what <script> tags do in HTML?",
|
||||
"Here is some code: <div><script src='app.js'></script></div>",
|
||||
"<img onerror='alert(1)' src='x'>",
|
||||
"<iframe src='https://example.com'></iframe>",
|
||||
"The <style> and <script> elements are important in HTML",
|
||||
"<a href='javascript:void(0)'>click me</a>",
|
||||
]
|
||||
|
||||
for message in html_messages:
|
||||
# Should NOT raise HTTPException
|
||||
result = await guardrail.apply_guardrail(
|
||||
inputs={"texts": [message]},
|
||||
request_data={},
|
||||
input_type="request",
|
||||
)
|
||||
processed_texts = result.get("texts", [])
|
||||
assert len(processed_texts) == 1
|
||||
# Content should pass through unchanged (no HTML tags are patterns)
|
||||
assert processed_texts[0] == message, (
|
||||
f"Message containing HTML was unexpectedly modified: "
|
||||
f"input={message!r}, output={processed_texts[0]!r}"
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_script_tag_not_blocked_with_blocked_words(self):
|
||||
"""
|
||||
Test that <script> tags are not accidentally caught by blocked words
|
||||
unless explicitly configured.
|
||||
|
||||
Regression test for GitHub issue #20441.
|
||||
"""
|
||||
blocked_words = [
|
||||
BlockedWord(
|
||||
keyword="confidential",
|
||||
action=ContentFilterAction.BLOCK,
|
||||
),
|
||||
BlockedWord(
|
||||
keyword="secret_project",
|
||||
action=ContentFilterAction.BLOCK,
|
||||
),
|
||||
]
|
||||
|
||||
guardrail = ContentFilterGuardrail(
|
||||
guardrail_name="test-script-not-blocked",
|
||||
blocked_words=blocked_words,
|
||||
)
|
||||
|
||||
# <script> should not be caught by unrelated blocked words
|
||||
script_messages = [
|
||||
"<script>alert('test')</script>",
|
||||
"How do I use <script> tags in HTML?",
|
||||
"<script src='app.js'></script>",
|
||||
]
|
||||
|
||||
for message in script_messages:
|
||||
result = await guardrail.apply_guardrail(
|
||||
inputs={"texts": [message]},
|
||||
request_data={},
|
||||
input_type="request",
|
||||
)
|
||||
processed_texts = result.get("texts", [])
|
||||
assert len(processed_texts) == 1
|
||||
assert processed_texts[0] == message
|
||||
|
||||
def test_no_builtin_pattern_matches_script_tag(self):
|
||||
"""
|
||||
Test that NONE of the prebuilt patterns in patterns.json match
|
||||
the string '<script>' or common HTML tags.
|
||||
|
||||
This is a safeguard to ensure that future pattern additions
|
||||
do not accidentally block legitimate LLM content containing
|
||||
HTML/code snippets.
|
||||
|
||||
Regression test for GitHub issue #20441.
|
||||
"""
|
||||
from litellm.proxy.guardrails.guardrail_hooks.litellm_content_filter.patterns import (
|
||||
PREBUILT_PATTERNS,
|
||||
get_compiled_pattern,
|
||||
)
|
||||
|
||||
html_test_strings = [
|
||||
"<script>alert('xss')</script>",
|
||||
"<script> test </script>",
|
||||
"<script src='app.js'></script>",
|
||||
"<img onerror='alert(1)' src='x'>",
|
||||
"<iframe src='https://example.com'></iframe>",
|
||||
"<style>body { color: red; }</style>",
|
||||
"<div onclick='alert(1)'>click</div>",
|
||||
]
|
||||
|
||||
for pattern_name in PREBUILT_PATTERNS:
|
||||
compiled = get_compiled_pattern(pattern_name)
|
||||
for test_string in html_test_strings:
|
||||
match = compiled.search(test_string)
|
||||
if match:
|
||||
# Some patterns may legitimately match substrings
|
||||
# (e.g., URL pattern matching src='https://...')
|
||||
# but they should not match the script/HTML tag itself
|
||||
matched_text = match.group()
|
||||
assert "<script" not in matched_text.lower(), (
|
||||
f"Pattern '{pattern_name}' matched '<script>' in "
|
||||
f"test string: {test_string!r}. "
|
||||
f"LLM message content should not be blocked for HTML tags."
|
||||
)
|
||||
|
|
|
|||
|
|
@ -0,0 +1,162 @@
|
|||
"""
|
||||
Tests for litellm/proxy/management_endpoints/common_utils.py
|
||||
|
||||
Covers the fix for GitHub issue #20304:
|
||||
Empty guardrails/policies arrays sent by the UI should NOT trigger the
|
||||
enterprise (premium) license check, but should still be applied so that
|
||||
users can intentionally clear previously-set fields.
|
||||
"""
|
||||
|
||||
from unittest.mock import patch
|
||||
|
||||
from litellm.proxy.management_endpoints.common_utils import (
|
||||
_update_metadata_fields,
|
||||
)
|
||||
|
||||
|
||||
class TestUpdateMetadataFieldsEmptyCollections:
|
||||
"""
|
||||
Regression tests for issue #20304.
|
||||
|
||||
The UI sends empty arrays (`[]`) for enterprise-only fields like
|
||||
guardrails, policies, and logging even when the user hasn't configured
|
||||
these features. The backend must not treat empty collections as an
|
||||
intent to use the feature, and therefore must not trigger the premium
|
||||
license check.
|
||||
|
||||
However, empty collections must still be written into metadata so that
|
||||
users can intentionally clear a previously-set field (e.g. removing all
|
||||
guardrails by sending `guardrails: []`).
|
||||
"""
|
||||
|
||||
@patch("litellm.proxy.management_endpoints.common_utils._premium_user_check")
|
||||
def test_empty_list_does_not_trigger_premium_check(self, mock_premium_check):
|
||||
"""Empty lists for premium fields must not trigger the premium check."""
|
||||
updated_kv = {
|
||||
"team_id": "test-team",
|
||||
"guardrails": [],
|
||||
"policies": [],
|
||||
"logging": [],
|
||||
}
|
||||
_update_metadata_fields(updated_kv=updated_kv)
|
||||
mock_premium_check.assert_not_called()
|
||||
|
||||
@patch("litellm.proxy.management_endpoints.common_utils._premium_user_check")
|
||||
def test_empty_list_still_updates_metadata(self, mock_premium_check):
|
||||
"""
|
||||
Empty lists must still be moved into metadata so users can clear
|
||||
previously-set fields (e.g. remove all guardrails).
|
||||
"""
|
||||
updated_kv = {
|
||||
"team_id": "test-team",
|
||||
"guardrails": [],
|
||||
"policies": [],
|
||||
}
|
||||
_update_metadata_fields(updated_kv=updated_kv)
|
||||
# The fields should have been moved into metadata
|
||||
assert "guardrails" not in updated_kv, (
|
||||
"guardrails should be popped from top-level"
|
||||
)
|
||||
assert "policies" not in updated_kv, (
|
||||
"policies should be popped from top-level"
|
||||
)
|
||||
assert updated_kv["metadata"]["guardrails"] == []
|
||||
assert updated_kv["metadata"]["policies"] == []
|
||||
|
||||
@patch("litellm.proxy.management_endpoints.common_utils._premium_user_check")
|
||||
def test_empty_dict_does_not_trigger_premium_check(self, mock_premium_check):
|
||||
"""Empty dicts for premium fields must not trigger the premium check."""
|
||||
updated_kv = {
|
||||
"team_id": "test-team",
|
||||
"secret_manager_settings": {},
|
||||
}
|
||||
_update_metadata_fields(updated_kv=updated_kv)
|
||||
mock_premium_check.assert_not_called()
|
||||
|
||||
@patch("litellm.proxy.management_endpoints.common_utils._premium_user_check")
|
||||
def test_empty_dict_still_updates_metadata(self, mock_premium_check):
|
||||
"""
|
||||
Empty dicts must still be moved into metadata so users can clear
|
||||
previously-set fields.
|
||||
"""
|
||||
updated_kv = {
|
||||
"team_id": "test-team",
|
||||
"secret_manager_settings": {},
|
||||
}
|
||||
_update_metadata_fields(updated_kv=updated_kv)
|
||||
assert "secret_manager_settings" not in updated_kv, (
|
||||
"secret_manager_settings should be popped from top-level"
|
||||
)
|
||||
assert updated_kv["metadata"]["secret_manager_settings"] == {}
|
||||
|
||||
@patch("litellm.proxy.management_endpoints.common_utils._premium_user_check")
|
||||
def test_none_value_does_not_trigger_premium_check(self, mock_premium_check):
|
||||
"""None values for premium fields should be silently ignored."""
|
||||
updated_kv = {
|
||||
"team_id": "test-team",
|
||||
"guardrails": None,
|
||||
"policies": None,
|
||||
}
|
||||
_update_metadata_fields(updated_kv=updated_kv)
|
||||
mock_premium_check.assert_not_called()
|
||||
|
||||
@patch("litellm.proxy.management_endpoints.common_utils._premium_user_check")
|
||||
def test_absent_fields_do_not_trigger_premium_check(self, mock_premium_check):
|
||||
"""Fields not present in the dict should not trigger premium check."""
|
||||
updated_kv = {
|
||||
"team_id": "test-team",
|
||||
"team_alias": "example-team",
|
||||
}
|
||||
_update_metadata_fields(updated_kv=updated_kv)
|
||||
mock_premium_check.assert_not_called()
|
||||
|
||||
@patch("litellm.proxy.management_endpoints.common_utils._premium_user_check")
|
||||
def test_non_empty_list_triggers_premium_check(self, mock_premium_check):
|
||||
"""Non-empty lists for premium fields should trigger the premium check."""
|
||||
updated_kv = {
|
||||
"team_id": "test-team",
|
||||
"guardrails": ["my-guardrail"],
|
||||
}
|
||||
_update_metadata_fields(updated_kv=updated_kv)
|
||||
mock_premium_check.assert_called()
|
||||
|
||||
@patch("litellm.proxy.management_endpoints.common_utils._premium_user_check")
|
||||
def test_non_empty_value_triggers_premium_check(self, mock_premium_check):
|
||||
"""Non-empty string values for premium fields should trigger the premium check."""
|
||||
updated_kv = {
|
||||
"team_id": "test-team",
|
||||
"tags": ["production"],
|
||||
}
|
||||
_update_metadata_fields(updated_kv=updated_kv)
|
||||
mock_premium_check.assert_called()
|
||||
|
||||
@patch("litellm.proxy.management_endpoints.common_utils._premium_user_check")
|
||||
def test_non_empty_list_updates_metadata(self, mock_premium_check):
|
||||
"""Non-empty lists should be moved into metadata."""
|
||||
updated_kv = {
|
||||
"team_id": "test-team",
|
||||
"guardrails": ["my-guardrail"],
|
||||
}
|
||||
_update_metadata_fields(updated_kv=updated_kv)
|
||||
assert "guardrails" not in updated_kv
|
||||
assert updated_kv["metadata"]["guardrails"] == ["my-guardrail"]
|
||||
|
||||
@patch("litellm.proxy.management_endpoints.common_utils._premium_user_check")
|
||||
def test_ui_typical_payload_does_not_trigger_premium_check(self, mock_premium_check):
|
||||
"""
|
||||
Simulate the exact payload the UI sends when no enterprise features
|
||||
are configured. This must NOT trigger the premium check.
|
||||
"""
|
||||
# This is the payload structure the UI sends (from issue #20304)
|
||||
updated_kv = {
|
||||
"team_id": "67848772-1a8b-4343-938c-17e60f1db860",
|
||||
"team_alias": "example-team",
|
||||
"models": ["gpt-4"],
|
||||
"metadata": {
|
||||
"guardrails": [],
|
||||
"logging": [],
|
||||
},
|
||||
"policies": [],
|
||||
}
|
||||
_update_metadata_fields(updated_kv=updated_kv)
|
||||
mock_premium_check.assert_not_called()
|
||||
|
|
@ -460,12 +460,12 @@ const TeamInfoView: React.FC<TeamInfoProps> = ({
|
|||
budget_duration: values.budget_duration,
|
||||
metadata: {
|
||||
...parsedMetadata,
|
||||
guardrails: values.guardrails || [],
|
||||
logging: values.logging_settings || [],
|
||||
...(values.guardrails?.length > 0 ? { guardrails: values.guardrails } : {}),
|
||||
...(values.logging_settings?.length > 0 ? { logging: values.logging_settings } : {}),
|
||||
disable_global_guardrails: values.disable_global_guardrails || false,
|
||||
...(secretManagerSettings !== undefined ? { secret_manager_settings: secretManagerSettings } : {}),
|
||||
},
|
||||
policies: values.policies || [],
|
||||
...(values.policies?.length > 0 ? { policies: values.policies } : {}),
|
||||
organization_id: values.organization_id,
|
||||
};
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue