feat(add-new-block_code_execution-guardrail): prevent agent from executing code - #22056
feat(add-new-block_code_execution-guardrail): prevent agent from executing code#22056ghost wants to merge 18 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryAdds a new
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/guardrails/guardrail_hooks/block_code_execution/block_code_execution.py | Core guardrail implementation (636 lines). Has overly broad phrase lists enabling both false positives and bypass vectors; _normalize_escaped_newlines corrupts legitimate content; no-execution short-circuit lacks conflict resolution with execution-intent phrases; regex misses \r\n newlines. |
| litellm/proxy/guardrails/guardrail_hooks/block_code_execution/init.py | Initialization and registration module. Clean implementation with proper config extraction and callback registration. No significant issues. |
| litellm/types/proxy/guardrails/guardrail_hooks/block_code_execution.py | Type definitions and config model for the guardrail. BLOCKED_LANGUAGES_OPTIONS is missing typescript and common aliases aren't documented for the UI. |
| litellm/types/guardrails.py | Added BLOCK_CODE_EXECUTION enum member, MULTISELECT/PERCENTAGE UI types, and config model to LitellmParams. Clean integration following existing patterns. |
| litellm/proxy/guardrails/guardrail_endpoints.py | Added _extract_literal_values for select fields, min/max/step propagation for percentage inputs, and ui_type string handling. Import reformatting is inconsistent with Black style. |
| tests/test_litellm/proxy/guardrails/guardrail_hooks/test_block_code_execution.py | Good test coverage (489 lines) with unit tests for detection, blocking, masking, escaped newlines, response-side blocking, and no-execution intent. All tests are mock-only with no network calls. |
| tests/test_litellm/proxy/guardrails/guardrail_hooks/test_block_code_execution_compliance.py | Compliance test runner against a 502-entry dataset. No network calls; validates 100% pass rate against the JSON dataset. |
| ui/litellm-dashboard/src/components/guardrails/guardrail_provider_fields.tsx | Added Slider component for percentage-type fields with min/max/step marks. Clean integration with existing form rendering. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Request/Response Text] --> B[_normalize_escaped_newlines]
B --> C{input_type?}
C -->|request| D{_has_no_execution_intent?}
C -->|response| F[_find_blocks - regex scan]
D -->|yes| E[Return text unchanged - ALLOW]
D -->|no| F
F --> G{blocks found?}
G -->|no, request| H{_has_execution_intent?}
G -->|no, response| E
G -->|yes| I[For each block: check language + confidence]
H -->|yes, action=block| J[BLOCK - execution request]
H -->|no| E
I --> K{effective_block?}
K -->|response| L[Always enforce block/mask]
K -->|request + detect_intent| M{has_execution_intent?}
K -->|request, no detect| L
M -->|yes| L
M -->|no| N[Log only / Allow]
L --> O{action?}
O -->|block| P[Raise HTTPException / ModifyResponseException]
O -->|mask| Q[Replace with CODE_BLOCK_REDACTED]
Last reviewed commit: 490beb5
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @log_guardrail_information | ||
| async def apply_guardrail( | ||
| self, | ||
| inputs: GenericGuardrailAPIInputs, | ||
| request_data: dict, | ||
| input_type: Literal["request", "response"], | ||
| logging_obj: Optional["LiteLLMLoggingObj"] = None, | ||
| ) -> GenericGuardrailAPIInputs: | ||
| start_time = datetime.now() | ||
| detections: List[CodeBlockDetection] = [] | ||
| status: GuardrailStatus = "success" | ||
| exception_str = "" | ||
|
|
||
| try: | ||
| texts = inputs.get("texts", []) | ||
| if not texts: | ||
| return inputs | ||
|
|
||
| is_output = input_type == "response" | ||
| processed: List[str] = [] | ||
| for text in texts: | ||
| new_text, should_raise = self._scan_text(text, detections) | ||
| processed.append(new_text) | ||
| if should_raise: | ||
| # Determine language from first blocking detection | ||
| lang = "unknown" | ||
| for d in detections: | ||
| if d.get("action_taken") == "block": | ||
| lang = d.get("language", "unknown") | ||
| break | ||
| self._raise_block_error(lang, is_output, request_data) | ||
|
|
||
| inputs["texts"] = processed | ||
| return inputs | ||
| except HTTPException: | ||
| status = "guardrail_intervened" | ||
| raise | ||
| except Exception as e: | ||
| status = "guardrail_failed_to_respond" | ||
| exception_str = str(e) | ||
| raise | ||
| finally: | ||
| guardrail_response: Union[List[dict], str] = [dict(d) for d in detections] | ||
| if status != "success" and not detections: | ||
| guardrail_response = exception_str | ||
| max_confidence: Optional[float] = None | ||
| for d in detections: | ||
| c = d.get("confidence") | ||
| if c is not None and (max_confidence is None or c > max_confidence): | ||
| max_confidence = c | ||
| tracing_kw: Dict[str, Any] = { | ||
| "guardrail_id": self.guardrail_name, | ||
| "detection_method": "fenced_code_block", | ||
| "match_details": guardrail_response, | ||
| } | ||
| if max_confidence is not None: | ||
| tracing_kw["confidence_score"] = max_confidence | ||
| self.add_standard_logging_guardrail_information_to_request_data( | ||
| guardrail_provider="block_code_execution", | ||
| guardrail_json_response=guardrail_response, | ||
| request_data=request_data, | ||
| guardrail_status=status, | ||
| start_time=start_time.timestamp(), | ||
| end_time=datetime.now().timestamp(), | ||
| duration=(datetime.now() - start_time).total_seconds(), | ||
| tracing_detail=GuardrailTracingDetail(**tracing_kw), # type: ignore[typeddict-item] | ||
| ) |
There was a problem hiding this comment.
Double-logging of guardrail information
The @log_guardrail_information decorator already calls self._process_response() on success and self._process_error() on exception — both of which internally call self.add_standard_logging_guardrail_information_to_request_data(). The finally block at line 308 also calls self.add_standard_logging_guardrail_information_to_request_data() directly, causing guardrail information to be appended to request_data["metadata"]["standard_logging_guardrail_information"] twice per invocation.
Other guardrails in the codebase use one pattern or the other, but never both:
- Decorator-only (e.g.
PresidioPIIMasking.apply_guardrail): uses@log_guardrail_informationand does not manually calladd_standard_logging_guardrail_information_to_request_data. - Manual-only (e.g.
ContentFilterGuardrail.apply_guardrail): does NOT use@log_guardrail_informationand manually callsadd_standard_logging_guardrail_information_to_request_datainfinally.
Either remove the @log_guardrail_information decorator and keep the manual finally block logging, or remove the finally block and rely on the decorator.
| async def async_post_call_streaming_iterator_hook( | ||
| self, | ||
| user_api_key_dict: Any, | ||
| response: Any, | ||
| request_data: dict, | ||
| ) -> AsyncGenerator[ModelResponseStream, None]: | ||
| """Accumulate streamed content and block if a complete fenced code block is detected.""" | ||
| accumulated = "" | ||
| async for item in response: | ||
| if isinstance(item, ModelResponseStream) and item.choices: | ||
| delta_content = "" | ||
| is_final = False | ||
| for choice in item.choices: | ||
| if hasattr(choice, "delta") and choice.delta: | ||
| content = getattr(choice.delta, "content", None) | ||
| if content and isinstance(content, str): | ||
| delta_content += content | ||
| if getattr(choice, "finish_reason", None): | ||
| is_final = True | ||
| accumulated += delta_content | ||
| if is_final: | ||
| # Run detection on full accumulated text (streaming: block only, no mask) | ||
| blocks = self._find_blocks(accumulated) | ||
| for _tag, _body, confidence, action_taken in blocks: | ||
| if action_taken == "block" and confidence >= self.confidence_threshold: | ||
| lang = _tag or "unknown" | ||
| self._raise_block_error(lang, True, request_data) | ||
| yield item |
There was a problem hiding this comment.
Streaming hook yields blocked content before raising error
async_post_call_streaming_iterator_hook yields every chunk immediately (yield item at line 362) and only checks for blocked code blocks when is_final is True. This means all streamed chunks containing the blocked code are already sent to the client before the error is raised. By the time the guardrail detects the blocked code at end-of-stream, the content has already been yielded.
To actually prevent the client from receiving blocked content during streaming, the hook would need to buffer chunks and only yield them after confirming they don't contain (or complete) a blocked code block — or at minimum, not yield the final chunk and raise before it.
| def _scan_text( | ||
| self, | ||
| text: str, | ||
| detections: Optional[List[CodeBlockDetection]] = None, | ||
| ) -> Tuple[str, bool]: | ||
| """ | ||
| Scan one text: find blocks, apply block/mask/allow by confidence. | ||
| Returns (modified_text, should_raise). | ||
| """ | ||
| if not text: | ||
| return text, False | ||
| blocks = self._find_blocks(text) | ||
| if not blocks: | ||
| return text, False | ||
|
|
||
| should_raise = False | ||
| last_end = 0 | ||
| parts: List[str] = [] | ||
| for m in FENCED_BLOCK_RE.finditer(text): | ||
| tag = (m.group(1) or "").strip() | ||
| tag_in_list = not self.block_all and _normalize_language(tag) in [ | ||
| _normalize_language(t) for t in (self.blocked_languages or []) | ||
| ] | ||
| is_blocked = _is_blocked_language( | ||
| tag, self.blocked_languages, self.block_all | ||
| ) | ||
| confidence = _confidence_for_block(tag, self.block_all, tag_in_list) | ||
| if not is_blocked: | ||
| action_taken: CodeBlockActionTaken = "allow" | ||
| elif confidence >= self.confidence_threshold: | ||
| action_taken = "block" | ||
| else: | ||
| action_taken = "log_only" | ||
|
|
||
| if detections is not None: | ||
| detections.append( | ||
| cast( | ||
| CodeBlockDetection, | ||
| { | ||
| "type": "code_block", | ||
| "language": tag or "(none)", | ||
| "confidence": round(confidence, 2), | ||
| "action_taken": action_taken, | ||
| }, | ||
| ) | ||
| ) | ||
|
|
||
| if action_taken == "block" and self.action == "block": | ||
| should_raise = True | ||
| parts.append(text[last_end : m.start()]) | ||
| if action_taken == "block": | ||
| parts.append(self.MASK_PLACEHOLDER) | ||
| else: | ||
| parts.append(text[m.start() : m.end()]) | ||
| last_end = m.end() | ||
|
|
||
| parts.append(text[last_end:]) | ||
| new_text = "".join(parts) | ||
| return new_text, should_raise |
There was a problem hiding this comment.
Redundant double regex iteration in _scan_text
_scan_text calls self._find_blocks(text) at line 199, which iterates over all FENCED_BLOCK_RE matches and computes tag_in_list, is_blocked, confidence, and action_taken for each match. Then at line 206 the method iterates FENCED_BLOCK_RE.finditer(text) again and recomputes the exact same values. The blocks result from _find_blocks is only used as an early-return check (if not blocks).
Consider removing the _find_blocks call and using the inline regex iteration directly, or reusing the results from _find_blocks instead of running the regex a second time.
|
@greptile please re-review |
| DEFAULT_BLOCKED_LANGUAGES: List[str] = [ | ||
| "python", | ||
| "javascript", | ||
| "js", | ||
| "bash", | ||
| "sh", | ||
| "ruby", | ||
| "go", | ||
| "java", | ||
| "csharp", | ||
| "php", | ||
| "c", | ||
| "cpp", | ||
| "rust", | ||
| "sql", | ||
| ] |
There was a problem hiding this comment.
DEFAULT_BLOCKED_LANGUAGES is defined but never used
The DEFAULT_BLOCKED_LANGUAGES list (lines 30-45) is never referenced anywhere in the codebase. When blocked_languages is None or empty, the guardrail uses block_all = True mode rather than falling back to this list. Consider removing this dead code or using it as the default for BLOCKED_LANGUAGES_OPTIONS in the config model to keep a single source of truth.
…block_code_execution.py Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
@greptile please re-review |
| except ModifyResponseException: | ||
| status = "guardrail_intervened" | ||
| raise |
There was a problem hiding this comment.
Missing import causes NameError at runtime
ModifyResponseException is caught at line 320 but is never imported in this file. When input_type == "request", _raise_block_error calls self.raise_passthrough_exception() which raises ModifyResponseException. Since the name is not in scope, Python will raise a NameError instead, which falls into the generic except Exception block at line 323 — misclassifying the intentional guardrail intervention as "guardrail_failed_to_respond".
The test file (test_block_code_execution.py:6) correctly imports it, but the implementation file does not. Add the import alongside CustomGuardrail:
| except ModifyResponseException: | |
| status = "guardrail_intervened" | |
| raise | |
| except ModifyResponseException: | |
| status = "guardrail_intervened" | |
| raise |
Also add at the top of the file (line 16):
from litellm.integrations.custom_guardrail import CustomGuardrail, ModifyResponseException| def _is_blocked_language( | ||
| tag: str, | ||
| blocked_languages: Optional[List[str]], | ||
| block_all: bool, | ||
| ) -> bool: | ||
| """True if this language tag should be considered blocked.""" | ||
| normalized = _normalize_language(tag) | ||
| if block_all: | ||
| # Block all: only allow through if it's explicitly non-executable (we still block but with lower confidence) | ||
| return True | ||
| if not blocked_languages: | ||
| return True | ||
| normalized_list = [_normalize_language(t) for t in blocked_languages] | ||
| return normalized in normalized_list |
There was a problem hiding this comment.
Unreachable code path when block_all=False and blocked_languages is empty
When block_all is False, the blocked_languages list is guaranteed to be non-empty (because block_all is set to True when blocked_languages is None or len(blocked_languages) == 0 in __init__ at line 166). This means the check at line 93 (if not blocked_languages: return True) is dead code — it can never be reached when block_all is False.
This isn't a runtime bug, but it makes the logic harder to reason about. Consider removing the dead branch or adding a comment explaining the invariant.
| def _normalize_escaped_newlines(text: str) -> str: | ||
| """ | ||
| Replace literal escaped newlines (backslash + n or backslash + r) with real newlines. | ||
| API/JSON payloads sometimes deliver newlines as the two-character sequence \\n. | ||
| """ | ||
| if not text: | ||
| return text | ||
| # Order matters: replace \r\n first so we don't produce extra \n from \r then \n | ||
| text = text.replace("\\r\\n", "\n") | ||
| text = text.replace("\\n", "\n") | ||
| text = text.replace("\\r", "\n") | ||
| return text |
There was a problem hiding this comment.
_normalize_escaped_newlines may corrupt legitimate content
This function unconditionally replaces the literal two-character sequence \n with a real newline in all text. This is problematic because:
- LLM responses can legitimately contain the literal string
\nwhen discussing code, escape sequences, or regex patterns (e.g., explaining"use \\n for newlines") - JSON-decoded strings would already have real newlines — the double-escaped
\\n→ literal\nscenario only occurs when text was not properly decoded from JSON
Consider applying this normalization more selectively — for example, only when the text appears to not contain any real newlines but does contain literal \n sequences, which would indicate it was passed as a raw JSON-encoded string.
|
@greptile can you please re-review? |
| for _start, _end, _tag, _body, confidence, action_taken in blocks: | ||
| if ( | ||
| action_taken == "block" | ||
| and confidence >= self.confidence_threshold | ||
| ): |
There was a problem hiding this comment.
Redundant confidence check in streaming hook
The confidence >= self.confidence_threshold check here is redundant. _find_blocks at line 188 already ensures that action_taken is set to "block" only when confidence >= self.confidence_threshold. If action_taken == "block", the confidence condition is guaranteed to be true.
| for _start, _end, _tag, _body, confidence, action_taken in blocks: | |
| if ( | |
| action_taken == "block" | |
| and confidence >= self.confidence_threshold | |
| ): | |
| if action_taken == "block": |
| if "\n" in text: | ||
| return text |
There was a problem hiding this comment.
Mixed real/escaped newlines bypass detection
If text contains both real newlines and literal \n sequences (e.g., "line1\nline2\\nline3"), this early return skips normalization entirely. A code block that uses literal \n within otherwise normal text would not be detected. Consider removing this early return or changing the heuristic to only skip when there are no \\n sequences present.
| if "\n" in text: | |
| return text | |
| if "\n" in text and "\\n" not in text: | |
| return text |
| accumulated += delta_content | ||
| # Check after every chunk so we block before yielding the chunk that completes a blocked block | ||
| blocks = self._find_blocks(accumulated) |
There was a problem hiding this comment.
Streaming hook skips escaped newline normalization
_scan_text applies _normalize_escaped_newlines before calling _find_blocks, but the streaming hook calls _find_blocks directly on the raw accumulated text. If a streaming response contains literal \n sequences (e.g., from a JSON-encoded payload), the regex won't match and the code block won't be detected.
Consider normalizing before the check:
normalized = _normalize_escaped_newlines(accumulated)
blocks = self._find_blocks(normalized)
Additional Comments (1)
|
|
@greptile please re-review |
| # Language tag aliases (normalize to canonical for comparison) | ||
| LANGUAGE_ALIASES: Dict[str, str] = { | ||
| "js": "javascript", | ||
| "py": "python", | ||
| } |
There was a problem hiding this comment.
Missing sh/bash and ts/typescript aliases causes detection gaps
LANGUAGE_ALIASES only maps js → javascript and py → python, but BLOCKED_LANGUAGES_OPTIONS in the config model offers both "bash" and "sh" as separate options. Since "sh" is not aliased to "bash" (or vice versa), a user who selects only "bash" will not block ```sh code blocks, and vice versa. The same gap exists for typescript/ts.
For example, if blocked_languages = ["bash"] and the LLM returns ```sh\nrm -rf /\n```, _normalize_language("sh") returns "sh" which is not in ["bash"] — so the block is allowed through.
| # Language tag aliases (normalize to canonical for comparison) | |
| LANGUAGE_ALIASES: Dict[str, str] = { | |
| "js": "javascript", | |
| "py": "python", | |
| } | |
| LANGUAGE_ALIASES: Dict[str, str] = { | |
| "js": "javascript", | |
| "py": "python", | |
| "sh": "bash", | |
| "ts": "typescript", | |
| } |
| async def async_post_call_streaming_iterator_hook( | ||
| self, | ||
| user_api_key_dict: Any, | ||
| response: Any, | ||
| request_data: dict, | ||
| ) -> AsyncGenerator[ModelResponseStream, None]: | ||
| """Accumulate streamed content and block as soon as a complete fenced code block is detected (before yielding that chunk).""" | ||
| accumulated = "" | ||
| async for item in response: | ||
| if isinstance(item, ModelResponseStream) and item.choices: | ||
| delta_content = "" | ||
| for choice in item.choices: | ||
| if hasattr(choice, "delta") and choice.delta: | ||
| content = getattr(choice.delta, "content", None) | ||
| if content and isinstance(content, str): | ||
| delta_content += content | ||
| accumulated += delta_content | ||
| # Check after every chunk so we block before yielding the chunk that completes a blocked block | ||
| normalized = _normalize_escaped_newlines(accumulated) | ||
| blocks = self._find_blocks(normalized) | ||
| for _start, _end, _tag, _body, confidence, action_taken in blocks: | ||
| if ( | ||
| action_taken == "block" | ||
| and confidence >= self.confidence_threshold | ||
| ): | ||
| lang = _tag or "unknown" | ||
| self._raise_block_error(lang, True, request_data) | ||
| yield item |
There was a problem hiding this comment.
Streaming hook does not log guardrail information
Unlike apply_guardrail which has a finally block that calls self.add_standard_logging_guardrail_information_to_request_data(...), the streaming hook never logs guardrail tracing data. When the guardrail blocks during streaming (raising HTTPException), there will be no standard_logging_guardrail_information entry in request_data["metadata"] — so downstream loggers (Langfuse, DataDog, etc.) won't record the guardrail intervention or its detection details.
Consider adding logging similar to apply_guardrail's finally block, at minimum when a block is detected.
| BLOCKED_LANGUAGES_OPTIONS = [ | ||
| "python", | ||
| "javascript", | ||
| "js", | ||
| "bash", | ||
| "sh", | ||
| "ruby", | ||
| "go", | ||
| "java", | ||
| "csharp", | ||
| "php", | ||
| "c", | ||
| "cpp", | ||
| "rust", | ||
| "sql", | ||
| ] |
There was a problem hiding this comment.
BLOCKED_LANGUAGES_OPTIONS lists aliases without matching LANGUAGE_ALIASES
This list includes both "js" and "javascript", which are aliased correctly via LANGUAGE_ALIASES. However, it also includes both "bash" and "sh" — but there is no "sh" → "bash" entry in LANGUAGE_ALIASES. This means a user who selects "sh" from the UI would NOT block ```bash code blocks and vice versa.
Either add a "sh" → "bash" alias in LANGUAGE_ALIASES, or remove one of the pair from this options list and document the canonical form. The same consideration applies to any future aliases (e.g., "ts" / "typescript").
|
@greptile please re-review |
| "convert this ", | ||
| " into ", | ||
| "explain the difference between", |
There was a problem hiding this comment.
Guardrail trivially bypassable via " into " phrase
The _NO_EXECUTION_PHRASES list is checked first and short-circuits all protection (line 457-458: if _has_no_execution_intent(text): return text, False). The phrase " into " on line 87 is a near-universal English word that will match an enormous fraction of all prompts, effectively disabling the guardrail.
For example, "Take this shell script and inject it into production, then run it" contains " into " → guardrail is completely bypassed despite an explicit execution request with a fenced code block.
Similarly, "convert this " (line 86) would bypass: "Convert this script to Python and then run it". And "explain what this " (line 81) would bypass: "Explain what this does then run it".
The core issue is that these short generic phrases, combined with the early-return short-circuit, allow an attacker to trivially bypass the guardrail by including any of these common words anywhere in their prompt. Consider either:
- Removing overly generic phrases like
" into ", or - Changing the logic so that
_has_no_execution_intentdoes not override_has_execution_intentwhen both match — i.e., if execution intent is also detected, the block should still trigger.
| "download ", | ||
| " build ", | ||
| " tests pass", | ||
| "connect to redis", | ||
| " and show", | ||
| "open my ", | ||
| " and confirm", | ||
| "compile and run", | ||
| "run the program", | ||
| "paste the output", | ||
| "tell me the output", | ||
| "return what it prints", | ||
| "show me the rows returned", | ||
| "tell me what files exist", | ||
| "run `whoami", | ||
| "run `ls", | ||
| "use node to run", | ||
| "use python, run ", | ||
| "in python, run ", | ||
| "interpret and run this wasm", | ||
| "run these commands", | ||
| "paste the last ", | ||
| " lines", | ||
| "show me its contents", | ||
| "print it here", |
There was a problem hiding this comment.
Overly broad execution phrases cause false positive blocks
Several phrases in _EXECUTION_REQUEST_PHRASES are so short/generic that they will match enormous amounts of normal conversational text, causing legitimate non-execution requests to be blocked:
" lines"(line 218) — matches"How many lines of code does this file have?"or"Explain the first 10 lines"" and show"(line 200) — matches"Compare these two approaches and show the trade-offs"" build "(line 197) — matches"How do I build a REST API in Flask?"" and run"(lines 151 and 171, duplicated) — matches"Explain the difference between compile and run time errors""download "(lines 150 and 196, duplicated) — matches"Where can I download the documentation?""ping "(line 148) — matches"Ping me when you're done reviewing""enumerate "(line 183) — matches"How do I enumerate a list in Python?""open a "(line 121) — matches"How do I open a file in Python?"
There are also duplicated entries: " and run" (151/171), "download " (150/196), "open my " (135/201), "run it anyway" (191/263), "run my notebook cell" (193/266), "attempt a reverse shell" (182/252), "paste the output" (205/233).
Since all matching is done via simple in substring containment, there are no word-boundary checks. Consider using regex with \b boundaries, removing the most generic phrases, or requiring multiple signals to trigger.
| def _normalize_escaped_newlines(text: str) -> str: | ||
| """ | ||
| Replace literal escaped newlines (backslash + n or backslash + r) with real newlines. | ||
| API/JSON payloads sometimes deliver newlines as the two-character sequence \\n. | ||
| Applied whenever \\n or \\r appear, including in mixed content with real newlines. | ||
| """ | ||
| if not text: | ||
| return text | ||
| if "\\n" not in text and "\\r" not in text: | ||
| return text | ||
| # Order matters: replace \r\n first so we don't produce extra \n from \r then \n | ||
| text = text.replace("\\r\\n", "\n") | ||
| text = text.replace("\\n", "\n") |
There was a problem hiding this comment.
_normalize_escaped_newlines corrupts legitimate \n literals
This function unconditionally replaces the literal two-character sequence \n with a real newline. LLM responses frequently discuss escape sequences and contain the literal string \n in explanatory text (e.g., "use \\n for newlines in Python"). After this normalization, such text will have its content altered — potentially breaking code explanations and creating phantom code block matches where fences were not intended.
For example, the text "In Python, print('hello\\nworld') outputs two lines" would be transformed to "In Python, print('hello\nworld') outputs two lines" — changing the semantic content of the response.
Consider applying this normalization more selectively — for example, only when the text contains no real newlines at all (suggesting it was passed as a raw JSON-encoded string), rather than always applying it.
| "resolveJsonModule": true, | ||
| "isolatedModules": true, | ||
| "jsx": "react-jsx", | ||
| "jsx": "preserve", |
There was a problem hiding this comment.
jsx: "preserve" may break build pipeline
Changing jsx from "react-jsx" to "preserve" means TypeScript will no longer transform JSX — it will be emitted as-is and must be handled by a downstream tool (e.g., Babel, SWC). If the existing Next.js build pipeline was configured to rely on TypeScript's JSX transformation, this change could cause build failures. Was this change intentional? If this is a Next.js project using SWC (the default), "preserve" is typically correct, but the change should be tested against the full build.
| "jsx": "preserve", | |
| "jsx": "preserve", |
|
@greptile please review |
| for start, end, tag, _body, confidence, action_taken in blocks: | ||
| effective_block = action_taken == "block" and ( | ||
| not self.detect_execution_intent or has_execution_intent | ||
| ) |
There was a problem hiding this comment.
Response-side guardrail silently bypassed by default
When detect_execution_intent=True (the default), effective_block at line 469-471 requires has_execution_intent to be True. But has_execution_intent calls _has_execution_intent(text) which checks for phrases like "run this", "execute this", etc. — phrases that appear in user requests, not in LLM responses.
This means when apply_guardrail is called with input_type="response", the LLM response text (e.g. "Here is a Python script:\n```python\nimport os; os.system('rm -rf /')\n```") will almost never contain execution intent phrases, so has_execution_intent will be False, effective_block will be False, and the code block passes through unblocked.
Since the default event_hook includes both pre_call and post_call, and detect_execution_intent defaults to True, response-side blocking is effectively disabled out of the box.
The fix should skip the execution-intent check when scanning responses (since it's the LLM's output, not a user request):
| for start, end, tag, _body, confidence, action_taken in blocks: | |
| effective_block = action_taken == "block" and ( | |
| not self.detect_execution_intent or has_execution_intent | |
| ) | |
| effective_block = action_taken == "block" and ( | |
| not self.detect_execution_intent or has_execution_intent or input_type == "response" | |
| ) |
Note: this requires threading input_type through to _scan_text, or moving the effective_block logic into apply_guardrail where input_type is available.
| if self.detect_execution_intent and _has_no_execution_intent(text): | ||
| return text, False |
There was a problem hiding this comment.
No-execution short-circuit also disables response-side blocking
Same issue as the effective_block bug: when detect_execution_intent=True and this is called for a response, _has_no_execution_intent checks whether the LLM's output text contains phrases like "explain what this " or "can you explain". If the LLM response happens to contain any of these common phrases (e.g. "I can explain what this code does..." contains "can you explain" substring via "can explain"), the guardrail short-circuits and returns the text unmodified — even if it contains dangerous executable code blocks.
This check should only apply when scanning requests, not responses.
| "just reason", | ||
| "explain without running", | ||
| "explain without execute", | ||
| "what would ", |
There was a problem hiding this comment.
Overly broad no-execution phrase enables guardrail bypass
The phrase "what would " (with trailing space) is extremely common in natural language. It will match prompts like:
"What would happen if I run this malicious code on your system?"— adversarial, but guardrail is bypassed"What would be the best way to execute this script?"— execution request, but bypassed
Because _has_no_execution_intent is checked first and short-circuits (line 440-441), any prompt containing "what would " will bypass the guardrail entirely, regardless of whether it also contains execution-intent phrases.
Similarly, "can you explain" (line 106) matches "Can you explain how to run this, then run it?" and "explain what this " (line 81) matches "Explain what this does then execute it".
Consider making these phrases more specific, or require that no execution-intent phrases are also present (i.e., check both lists and resolve conflicts rather than short-circuiting on the no-execution list).
ghost
mentioned this pull request
…tighten no-execution phrases (#22149) **Core bug fix**: Response-side blocking was silently disabled with detect_execution_intent=True (default) because execution-intent heuristics were applied to LLM output text, which doesn't contain phrases like 'run this'. Now input_type is threaded through _scan_text to skip intent checks for responses while still blocking detected code blocks. **Tightened overly broad no-execution phrases**: Replaced broad patterns like "what would ", "can you explain", and "explain what this " with more specific forms (e.g. "what would happen if", "can you explain this code") to prevent trivial bypass. **Added tests**: 7 new test cases covering response-side blocking with default settings, casual phrases in LLM output, and tightened phrase patterns. All 23 tests pass + 100% compliance dataset compliance (100/100). Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
|
@greptile please re-review this |
| from litellm.proxy.guardrails.guardrail_hooks.custom_code.code_validator import ( | ||
| CustomCodeValidationError, | ||
| validate_custom_code, | ||
| ) | ||
| from litellm.proxy.guardrails.guardrail_hooks.custom_code.primitives import ( | ||
| get_custom_code_primitives, | ||
| ) | ||
| from litellm.proxy.guardrails.usage_endpoints import router as guardrails_usage_router | ||
| from litellm.types.guardrails import ( | ||
| PII_ENTITY_CATEGORIES_MAP, | ||
| ApplyGuardrailRequest, | ||
| ApplyGuardrailResponse, | ||
| BaseLitellmParams, | ||
| BedrockGuardrailConfigModel, | ||
| Guardrail, | ||
| GuardrailEventHooks, | ||
| GuardrailInfoResponse, | ||
| GuardrailUIAddGuardrailSettings, | ||
| LakeraV2GuardrailConfigModel, | ||
| ListGuardrailsResponse, | ||
| LitellmParams, | ||
| PatchGuardrailRequest, | ||
| PiiAction, | ||
| PiiEntityType, | ||
| PresidioPresidioConfigModelUserInterface, | ||
| SupportedGuardrailIntegrations, | ||
| ToolPermissionGuardrailConfigModel, | ||
| ) | ||
| CustomCodeValidationError, validate_custom_code) | ||
| from litellm.proxy.guardrails.guardrail_hooks.custom_code.primitives import \ | ||
| get_custom_code_primitives | ||
| from litellm.proxy.guardrails.guardrail_registry import GuardrailRegistry | ||
| from litellm.proxy.guardrails.usage_endpoints import \ | ||
| router as guardrails_usage_router | ||
| from litellm.types.guardrails import (PII_ENTITY_CATEGORIES_MAP, | ||
| ApplyGuardrailRequest, | ||
| ApplyGuardrailResponse, | ||
| BaseLitellmParams, | ||
| BedrockGuardrailConfigModel, Guardrail, | ||
| GuardrailEventHooks, | ||
| GuardrailInfoResponse, | ||
| GuardrailUIAddGuardrailSettings, | ||
| LakeraV2GuardrailConfigModel, | ||
| ListGuardrailsResponse, LitellmParams, | ||
| PatchGuardrailRequest, PiiAction, | ||
| PiiEntityType, | ||
| PresidioPresidioConfigModelUserInterface, | ||
| SupportedGuardrailIntegrations, |
There was a problem hiding this comment.
Import style reformatted by non-standard formatter
The imports in this file were reformatted from the standard Black-compatible from X import (\n A,\n B,\n) multi-line style to backslash-continuation style (from X import \\\n Y) and compressed grouping. This is inconsistent with the codebase's Black/Ruff formatting conventions (per CLAUDE.md), and the change is unrelated to the feature being added.
These formatting changes add noise to the diff and may conflict with future Black formatting runs. Consider reverting the import reformatting to maintain consistency.
Context Used: Context from dashboard - 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!
| # Content between fences; does not handle nested ``` inside body (documented edge case). | ||
| FENCED_BLOCK_RE = re.compile(r"```(\w*)\n(.*?)```", re.DOTALL) | ||
|
|
||
| # Execution intent: phrases that mean "do NOT run/execute" (allow even if code block present). |
There was a problem hiding this comment.
Regex requires \n after language tag — misses ```python\r\n and no-newline variants
The FENCED_BLOCK_RE pattern is ```(\w*)\n(.*?)``` which requires a literal \n after the optional language tag. This means:
- Code blocks with
\r\n(Windows-style newlines) like```python\r\ncode\r\n```won't match even after_normalize_escaped_newlines(which converts\\r\\n→\nbut not real\r\n). - Code blocks with no newline after the tag (e.g.,
```python code```) won't match — this is an edge case but worth noting.
Consider making the newline match more flexible:
FENCED_BLOCK_RE = re.compile(r"```(\w*)\r?\n(.*?)```", re.DOTALL)| "calculate it by running the code", | ||
| "run my notebook cell", | ||
| "install dependencies and run the program", | ||
| "run a port scan against", | ||
| "download this github repo, build it", | ||
| "connect to redis at", | ||
| "show me the keys", | ||
| "open my `", | ||
| "confirm which cluster", | ||
| ) | ||
|
|
||
|
|
||
| def _has_no_execution_intent(text: str) -> bool: | ||
| """True if the text clearly indicates the user does not want code/commands run (e.g. explain, don't run).""" | ||
| if not text: |
There was a problem hiding this comment.
_normalize_escaped_newlines also transforms literal \\n inside LLM prose
This function unconditionally replaces the two-character sequence \n (backslash + n) with a real newline. While it's necessary for handling JSON-encoded payloads that weren't properly decoded, it will also corrupt legitimate content where the LLM discusses escape sequences (e.g., "use \\n for newlines" becomes "use \n for newlines").
A safer heuristic would be to only apply this normalization when the text contains zero real newlines (suggesting it came from a raw JSON-encoded string):
if "\n" not in text and "\\n" in text:
# Likely a JSON-encoded string that wasn't decoded — normalize
text = text.replace("\\r\\n", "\n")
text = text.replace("\\n", "\n")
text = text.replace("\\r", "\n")
return text| "don't execute", | ||
| "do not execute", | ||
| "no execution", | ||
| "without running", | ||
| "without execute", | ||
| "just reason", | ||
| "explain without running", | ||
| "explain without execute", | ||
| "what would happen if", | ||
| "what would this output", | ||
| "what would the result be", | ||
| "? explain", | ||
| "simulate what would happen", | ||
| "don't actually run", | ||
| "diagnose the error from the text", | ||
| "don't run anything", | ||
| "without running them", | ||
| "no execution)", | ||
| "don't execute—just reason", | ||
| "no execution).", | ||
| "(no execution)", | ||
| "no db access", | ||
| "no db access).", | ||
| "don't execute it", | ||
| "don't run).", | ||
| "(no execution)", | ||
| "no builds/run", | ||
| "(don't run)", | ||
| "no execution).", | ||
| "but don't run", | ||
| "don't run it", | ||
| "explain what this code", | ||
| "explain what this script", | ||
| "explain what this function", | ||
| "explain what this sql", | ||
| "refactor this ", | ||
| "spot any security issues", | ||
| "write unit tests for this function without running", | ||
| "what output *should* this produce", | ||
| "convert this ", | ||
| "explain the difference between", | ||
| "given this stack trace, explain", | ||
| "write a safe alternative", | ||
| "write a python function", | ||
| "generate a dockerfile", | ||
| "write a bash script that would", | ||
| "create a minimal ", | ||
| " example (no execution)", | ||
| "write pseudocode", | ||
| "generate typescript types", | ||
| "write a safe wrapper", | ||
| "show how to parse stdout", | ||
| "can you *simulate*", | ||
| "is this command safe to run", | ||
| "i pasted logs from", | ||
| "can you diagnose", | ||
| "what would `git", | ||
| "here's a traceback", | ||
| "can you explain this code", | ||
| "can you explain what this", | ||
| "can you explain how this works", | ||
| ) | ||
|
|
||
| # Execution intent: phrases that mean "run/execute/perform this for me" (block when on request). | ||
| # Used to block (1) requests that contain blocked fenced code + execution intent, and | ||
| # (2) requests with no fenced code but clear execution request (e.g. "run `ls`", "read /etc/passwd"). | ||
| _EXECUTION_REQUEST_PHRASES: Tuple[str, ...] = ( |
There was a problem hiding this comment.
_NO_EXECUTION_PHRASES contains overly broad and duplicate entries
Several entries in this list are problematic:
-
Overly broad:
"refactor this "(line 103),"convert this "(line 106),"create a minimal "(line 113) are very short generic phrases that can appear in prompts with execution intent (e.g.,"Refactor this and then run it","Convert this to Python and execute it"). Since these short-circuit all blocking via_has_no_execution_intent, they create easy bypass vectors. -
Duplicate entries:
"(no execution)"appears at lines 91, 95, and 98."no execution)."appears at lines 92 and 100. -
Trailing punctuation inconsistency: Some entries include trailing punctuation like
"no execution).","don't run).","no db access)."which will only match when the exact punctuation is present.
Consider tightening broad phrases to require more specific context (e.g., "refactor this code" instead of "refactor this ") and removing duplicates.
| "can you run ", | ||
| "run `", | ||
| "execute `", | ||
| " repl ", | ||
| "run this python", | ||
| "run this javascript", | ||
| "run this bash", | ||
| "run this node", | ||
| "run this sql", | ||
| "run this c ", | ||
| "run this powershell", | ||
| "run this ruby", | ||
| "run this lua", | ||
| "read `/", | ||
| "read /", | ||
| "open `", | ||
| "open my ", | ||
| "list the files", | ||
| "create a file", | ||
| "search my disk", | ||
| "cat the contents", | ||
| "write a script, execute it", | ||
| "check whether ", | ||
| " exists and paste", | ||
| "load `", | ||
| " and compute", | ||
| "curl ", | ||
| "make an http request", | ||
| "run `nslookup", | ||
| "fetch https", | ||
| "connect to postgres", | ||
| "hit this webhook", | ||
| "call aws ", | ||
| "query the dns", | ||
| "run subprocess", | ||
| "run `git", | ||
| "run `docker", | ||
| "execute `kubectl", | ||
| "run `terraform", | ||
| "run `npm test", | ||
| "run `pip install", | ||
| "run `pip show", | ||
| "run `make ", | ||
| "run `python setup", | ||
| "evaluate this ", | ||
| "run `eval", | ||
| "execute this lua", | ||
| "run this code", | ||
| "spin up ", | ||
| " and run", | ||
| "execute this user-provided", | ||
| "run this python that", | ||
| "try to escape", | ||
| "run commands to", | ||
| "attempt to mount", | ||
| "check if you have access to docker", | ||
| "probe for cloud metadata", | ||
| "run `sudo", | ||
| "try `cat", | ||
| "attempt a reverse shell", | ||
| " and send", | ||
| "access the system keychain", | ||
| "run code", | ||
| "execute it", | ||
| "run the snippet", | ||
| "execute the command", | ||
| "just run it", | ||
| "run it anyway", | ||
| "execute it to confirm", | ||
| "install dependencies and run", | ||
| "run a port scan", | ||
| " tests pass", | ||
| "connect to redis", | ||
| "compile and run", | ||
| "run the program", | ||
| "tell me the output", | ||
| "return what it prints", | ||
| "show me the rows returned", | ||
| "tell me what files exist", | ||
| "run `whoami", | ||
| "run `ls", | ||
| "use node to run", | ||
| "use python, run ", | ||
| "in python, run ", | ||
| "interpret and run this wasm", | ||
| "run these commands", | ||
| "paste the last ", | ||
| "show me its contents", | ||
| "print it here", | ||
| "show me where it is", | ||
| "tell me the dependencies", | ||
| "show me the variables", | ||
| "save the results to", | ||
| "confirm it exists", | ||
| "and paste the version", | ||
| "tell me the latency", | ||
| "tell me what it returns", | ||
| "show me what it returns", | ||
| "confirm it succeeded", | ||
| "tell me the account id", | ||
| "show it here", | ||
| "show me the response", | ||
| "paste the output", | ||
| "show me the containers", | ||
| "summarize the diff", | ||
| "tell me which tests fail", | ||
| "let me know if it errors", | ||
| "report back the output", | ||
| "paste the logs", | ||
| "tell me the result", | ||
| "show me the printed lines", | ||
| "tell me whether it calls", | ||
| "show me the stack trace", | ||
| "try to escape the sandbox", | ||
| "run commands to see if you can read", | ||
| "attempt to mount the root", | ||
| "check if you have access to docker socket", | ||
| "run a privileged container", | ||
| "probe for cloud metadata at", | ||
| "run `sudo -l`", | ||
| "try `cat /var/run/secrets", | ||
| "confirm it connects", | ||
| "send them to this url", | ||
| "list saved passwords", |
There was a problem hiding this comment.
_EXECUTION_REQUEST_PHRASES contains overly broad phrases causing false positives
Several phrases are so short or generic that they will match normal conversational text, blocking legitimate non-execution requests:
" and run"(line 176) — matches"Explain the difference between compile and run time errors"" tests pass"(line 231) — matches"How do I check if tests pass?"or"Do the tests pass in CI?""curl "(line 171) — matches"How does curl work?"or"Can you explain what a curl command does?""evaluate this "(line 182) — matches"Can you evaluate this approach?"(non-execution context)"check whether "(line 164) — matches"Can you check whether this logic is correct?"
These will cause false positive blocks, especially for users asking educational/explanatory questions about commands. Consider making phrases more specific (e.g., "run curl"instead of just"curl "`).
| is_response = input_type == "response" | ||
|
|
There was a problem hiding this comment.
_has_no_execution_intent short-circuits all protection without checking for conflicting execution intent
When _has_no_execution_intent(text) returns True (line 467), _scan_text returns early without checking for code blocks at all. But the function only checks if any no-execution phrase appears as a substring — it doesn't verify there are no conflicting execution-intent phrases.
This means a prompt like "Don't run this on staging, but run this on production:\n\``bash\nrm -rf /\n```"would bypass the guardrail because"don't run"matches_NO_EXECUTION_PHRASES`, even though the user explicitly requests execution elsewhere in the prompt.
Consider either:
- Checking both lists and requiring that no execution-intent phrases are present when a no-execution phrase is found
- Making the no-execution check only apply when no execution-intent phrases are also detected
| "execute `kubectl", | ||
| "run `terraform", | ||
| "run `npm test", | ||
| "run `pip install", | ||
| "run `pip show", | ||
| "run `make ", | ||
| "run `python setup", | ||
| "evaluate this ", | ||
| "run `eval", | ||
| "execute this lua", | ||
| "run this code", | ||
| "spin up ", |
There was a problem hiding this comment.
Duplicate " and run" and "download " entries
" and run" appears at both lines 176 and 199. "download " appears at both lines 150 and 220. These duplicates don't cause runtime errors but should be cleaned up:
| "execute `kubectl", | |
| "run `terraform", | |
| "run `npm test", | |
| "run `pip install", | |
| "run `pip show", | |
| "run `make ", | |
| "run `python setup", | |
| "evaluate this ", | |
| "run `eval", | |
| "execute this lua", | |
| "run this code", | |
| "spin up ", | |
| " and run", | |
| "execute this user-provided", | |
| "run this python that", | |
| "try to escape", | |
| "run commands to", | |
| "attempt to mount", | |
| "check if you have access to docker", | |
| "probe for cloud metadata", | |
| "run `sudo", | |
| "try `cat", | |
| "attempt a reverse shell", | |
| "access the system keychain", |
| BLOCKED_LANGUAGES_OPTIONS = [ | ||
| "python", | ||
| "javascript", | ||
| "bash", | ||
| "ruby", | ||
| "go", | ||
| "java", | ||
| "csharp", | ||
| "php", | ||
| "c", | ||
| "cpp", | ||
| "rust", | ||
| "sql", | ||
| ] |
There was a problem hiding this comment.
BLOCKED_LANGUAGES_OPTIONS missing common aliases like sh, ts, js, py
This list only includes canonical names (e.g., "python", "bash"), but LANGUAGE_ALIASES in the guardrail normalizes sh→bash, ts→typescript, js→javascript, py→python. However, BLOCKED_LANGUAGES_OPTIONS doesn't include "typescript" at all, and since the UI only shows these options, users cannot select TypeScript for blocking.
Consider either:
- Adding
"typescript"(and any other missing canonical names) to this list - Adding a note that common aliases like
sh,py,js,tsare automatically covered
ghost
mentioned this pull request
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewCI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes