fix(guardrails): address greptile review issues for CWE-022 path traversal fix

- Fix _assert_within_categories_dir try/except to only guard commonpath()
  so the intentional ValueError propagates with its full message
- Widen jail root from categories/ to module dir so policy_templates/
  files (EU AI Act, SG PDPA, etc.) continue to load without error
- Wrap _resolve_category_file_path call in _load_categories with
  try/except ValueError so a traversal attempt logs + skips instead
  of aborting ContentFilterGuardrail construction entirely
- Fix test_valid_category_file_inside_categories_dir_allowed to use
  pytest.skip instead of a silent if-guard that made the test a no-op
This commit is contained in:
Avani-prajapati 2026-05-19 18:13:27 +05:30
parent f176966be8
commit a5f02368df
No known key found for this signature in database
2 changed files with 27 additions and 20 deletions

View file

@ -333,15 +333,17 @@ class ContentFilterGuardrail(CustomGuardrail):
resolved = os.path.realpath(path)
allowed = os.path.realpath(categories_dir)
try:
if os.path.commonpath([resolved, allowed]) != allowed:
raise ValueError(
f"Category file path '{path}' is outside the allowed "
f"categories directory '{categories_dir}'"
)
except ValueError as exc:
common = os.path.commonpath([resolved, allowed])
except ValueError:
# commonpath() raises ValueError on Windows when paths span different drives
raise ValueError(
f"Category file path '{path}' is outside the allowed categories directory"
) from exc
)
if common != allowed:
raise ValueError(
f"Category file path '{path}' is outside the allowed "
f"categories directory '{categories_dir}'"
)
def _resolve_category_file_path(self, file_path: str) -> str:
"""
@ -354,7 +356,7 @@ class ContentFilterGuardrail(CustomGuardrail):
file isn't found.
Resolution order:
1. Return as-is if absolute or already exists (jailed to categories/).
1. Return as-is if absolute or already exists (jailed to module dir).
2. Try joining the full path relative to this module's directory (jailed).
3. Progressively strip leading path components and try each suffix
relative to this module's directory (jailed).
@ -367,20 +369,18 @@ class ContentFilterGuardrail(CustomGuardrail):
resolution fails (caller should check existence).
Raises:
ValueError: If the resolved path escapes the categories directory.
ValueError: If the resolved path escapes the module directory.
"""
categories_dir = os.path.join(os.path.dirname(__file__), "categories")
module_dir = os.path.dirname(__file__)
if os.path.isabs(file_path) or os.path.exists(file_path):
self._assert_within_categories_dir(file_path, categories_dir)
self._assert_within_categories_dir(file_path, module_dir)
return file_path
module_dir = os.path.dirname(__file__)
# Try the full relative path joined to the module directory
candidate = os.path.join(module_dir, file_path)
if os.path.exists(candidate):
self._assert_within_categories_dir(candidate, categories_dir)
self._assert_within_categories_dir(candidate, module_dir)
return candidate
# Progressively strip leading components to find a matching suffix
@ -389,14 +389,14 @@ class ContentFilterGuardrail(CustomGuardrail):
suffix = os.path.join(*parts[i:])
candidate = os.path.join(module_dir, suffix)
if os.path.exists(candidate):
self._assert_within_categories_dir(candidate, categories_dir)
self._assert_within_categories_dir(candidate, module_dir)
return candidate
# File not found via any resolution strategy — jail the module-relative
# path anyway to reject traversal attempts (e.g. "../../../../etc/passwd")
# regardless of CWD or whether the target file exists.
self._assert_within_categories_dir(
os.path.join(module_dir, file_path), categories_dir
os.path.join(module_dir, file_path), module_dir
)
return file_path
@ -445,7 +445,13 @@ class ContentFilterGuardrail(CustomGuardrail):
# Load category file (custom or default)
if custom_file:
category_file_path = self._resolve_category_file_path(custom_file)
try:
category_file_path = self._resolve_category_file_path(custom_file)
except ValueError as e:
verbose_proxy_logger.warning(
f"Category {category_name}: invalid category_file path, skipping. {e}"
)
continue
else:
# Try .yaml first, then .json (e.g. harm_toxic_abuse.json)
yaml_path = os.path.join(categories_dir, f"{category_name}.yaml")

View file

@ -34,9 +34,10 @@ class TestContentFilterPathTraversal:
"categories",
)
valid_file = os.path.join(categories_dir, "harmful_self_harm.yaml")
if os.path.exists(valid_file):
result = guardrail._resolve_category_file_path(valid_file)
assert result == valid_file
if not os.path.exists(valid_file):
pytest.skip("harmful_self_harm.yaml not present in this environment")
result = guardrail._resolve_category_file_path(valid_file)
assert result == valid_file
def test_invalid_category_name_skipped(self):
from litellm.proxy.guardrails.guardrail_hooks.litellm_content_filter.content_filter import (