From 9ebde9fb5c9d9f8fb43e4f73d0cef2a7c17ccc14 Mon Sep 17 00:00:00 2001 From: Krrish Dholakia Date: Wed, 11 Mar 2026 20:42:15 -0700 Subject: [PATCH] Address Greptile code review: response-only guard, fail-closed init, no matched_text in HTTP error, ReDoS protection - Add input_type == "response" guard so tone detection only fires on LLM output - Remove try/except in _init_tone_checker for fail-closed behavior on bad config - Remove matched_text from HTTP 400 error detail (keep in internal detection only) - Add _validate_user_pattern() with length limit and regex validation for ReDoS protection - Add tests for input_type guard, invalid regex, and pattern length limit Co-Authored-By: Claude Opus 4.6 --- .../litellm_content_filter/content_filter.py | 23 ++++------- .../tone_detection/base.py | 20 ++++++++++ .../guardrail_hooks/test_tone_detector.py | 40 +++++++++++++++++++ 3 files changed, 68 insertions(+), 15 deletions(-) diff --git a/litellm/proxy/guardrails/guardrail_hooks/litellm_content_filter/content_filter.py b/litellm/proxy/guardrails/guardrail_hooks/litellm_content_filter/content_filter.py index 34622973876..b8b208ae8b6 100644 --- a/litellm/proxy/guardrails/guardrail_hooks/litellm_content_filter/content_filter.py +++ b/litellm/proxy/guardrails/guardrail_hooks/litellm_content_filter/content_filter.py @@ -310,16 +310,10 @@ class ContentFilterGuardrail(CustomGuardrail): ) def _init_tone_checker(self, tone_detection_config: Dict[str, Any]) -> None: - try: - self._tone_checker = ToneChecker(tone_detection_config) - verbose_proxy_logger.debug( - "ContentFilterGuardrail: tone checker enabled" - ) - except Exception as e: - verbose_proxy_logger.warning( - "ContentFilterGuardrail: failed to init tone checker: %s", - e, - ) + self._tone_checker = ToneChecker(tone_detection_config) + verbose_proxy_logger.debug( + "ContentFilterGuardrail: tone checker enabled" + ) def _apply_tone_detection_policy( self, @@ -332,20 +326,19 @@ class ContentFilterGuardrail(CustomGuardrail): detection: ToneDetection = { "type": "tone", "category": category, + # matched_text kept in internal detection for logging/tracing only "matched_text": matched, } detections.append(detection) verbose_proxy_logger.warning( - "ContentFilterGuardrail: tone violation (%s): '%s'", + "ContentFilterGuardrail: tone violation (%s)", category, - matched, ) raise HTTPException( status_code=400, detail={ "error": f"Tone violation detected: {category}", "category": category, - "matched_text": matched, }, ) @@ -1869,8 +1862,8 @@ class ContentFilterGuardrail(CustomGuardrail): self._apply_competitor_intent_policy( intent_result, request_data, detections ) - # Tone detection (optional; raises on violation) - if self._tone_checker and text: + # Tone detection — only on LLM responses, not user input + if self._tone_checker and text and input_type == "response": tone_result = self._tone_checker.run(text) if tone_result is not None: self._apply_tone_detection_policy( diff --git a/litellm/proxy/guardrails/guardrail_hooks/litellm_content_filter/tone_detection/base.py b/litellm/proxy/guardrails/guardrail_hooks/litellm_content_filter/tone_detection/base.py index 21b1ca610d9..352d5565f3e 100644 --- a/litellm/proxy/guardrails/guardrail_hooks/litellm_content_filter/tone_detection/base.py +++ b/litellm/proxy/guardrails/guardrail_hooks/litellm_content_filter/tone_detection/base.py @@ -11,6 +11,9 @@ from typing import Dict, List, Optional, Pattern, Tuple from litellm._logging import verbose_proxy_logger +# Maximum length for user-supplied regex patterns to mitigate ReDoS +_MAX_PATTERN_LENGTH = 1024 + # --------------------------------------------------------------------------- # Built-in tone patterns — each is (raw_regex, category_label) # --------------------------------------------------------------------------- @@ -71,6 +74,21 @@ def _compile_patterns( _COMPILED_TONE_PATTERNS = _compile_patterns(_TONE_PATTERNS) +def _validate_user_pattern(pattern: str) -> None: + """Validate a user-supplied regex pattern for safety. + + Raises ValueError on invalid regex or patterns exceeding the length limit. + """ + if len(pattern) > _MAX_PATTERN_LENGTH: + raise ValueError( + f"Tone detection: pattern exceeds maximum length of {_MAX_PATTERN_LENGTH} characters" + ) + try: + re.compile(pattern) + except re.error as e: + raise ValueError(f"Tone detection: invalid regex pattern: {e}") from e + + class ToneChecker: """ CPU-only tone checker. @@ -83,12 +101,14 @@ class ToneChecker: def __init__(self, config: Dict) -> None: self._extra_blocked: List[Tuple[Pattern[str], str]] = [] for phrase in config.get("blocked_phrases") or []: + _validate_user_pattern(phrase) self._extra_blocked.append( (re.compile(phrase, re.IGNORECASE), "custom_blocked") ) self._safe_patterns: List[Pattern[str]] = [] for phrase in config.get("safe_phrases") or []: + _validate_user_pattern(phrase) self._safe_patterns.append(re.compile(phrase, re.IGNORECASE)) verbose_proxy_logger.debug( diff --git a/tests/test_litellm/proxy/guardrails/guardrail_hooks/test_tone_detector.py b/tests/test_litellm/proxy/guardrails/guardrail_hooks/test_tone_detector.py index f9dd1636016..c11471651ac 100644 --- a/tests/test_litellm/proxy/guardrails/guardrail_hooks/test_tone_detector.py +++ b/tests/test_litellm/proxy/guardrails/guardrail_hooks/test_tone_detector.py @@ -423,3 +423,43 @@ class TestInit: def test_tone_checker_disabled_when_no_config(self): g = ContentFilterGuardrail(guardrail_name="test-no-tone") assert g._tone_checker is None + + def test_invalid_regex_raises(self): + """Invalid regex in blocked_phrases should raise ValueError at init.""" + with pytest.raises(ValueError, match="invalid regex pattern"): + _make_guardrail(blocked_phrases=[r"(unclosed"]) + + def test_pattern_too_long_raises(self): + """Patterns exceeding the length limit should raise ValueError.""" + with pytest.raises(ValueError, match="exceeds maximum length"): + _make_guardrail(blocked_phrases=["a" * 2000]) + + +# --------------------------------------------------------------------------- +# INPUT TYPE GUARD — tone detection only fires on responses +# --------------------------------------------------------------------------- + +class TestInputTypeGuard: + + @pytest.mark.asyncio + async def test_request_input_type_skips_tone_detection(self): + """Tone detection should NOT fire when input_type is 'request'.""" + g = _make_guardrail() + # This text would be blocked on 'response' but should pass on 'request' + result = await g.apply_guardrail( + _inputs("That's not my problem."), + {}, + "request", + ) + assert result["texts"] == ["That's not my problem."] + + @pytest.mark.asyncio + async def test_response_input_type_fires_tone_detection(self): + """Tone detection should fire when input_type is 'response'.""" + g = _make_guardrail() + with pytest.raises(HTTPException): + await g.apply_guardrail( + _inputs("That's not my problem."), + {}, + "response", + )