fix(proxy): add shared path utilities, prevent directory traversal#25834
Conversation
Add safe_join() and safe_filename() in proxy/common_utils/path_utils.py for constructing filesystem paths from user-controlled inputs. Apply to guardrail category YAML endpoint and dotprompt file converter.
Add null byte rejection to safe_join and safe_filename. Normalize backslash separators in safe_filename for cross-platform safety. Include resolved path in ValueError for debugging. Move imports to module level per project conventions.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR introduces
Confidence Score: 5/5Safe to merge; the path traversal fixes are correct and no regressions introduced. All prior P1 concerns (missing tests, import ordering) have been addressed in follow-up commits. The only remaining finding is a P2 incorrect HTTP status code (500 instead of 400) for a null-byte filename in the dotprompt endpoint — a rejected request with the wrong status code, not a security or data-integrity issue. litellm/proxy/prompts/prompt_endpoints.py — the ValueError from safe_filename should return 400, not 500.
|
| Filename | Overview |
|---|---|
| litellm/proxy/common_utils/path_utils.py | New module providing safe_join() and safe_filename(); realpath-based containment and null-byte guards are correctly implemented. |
| litellm/proxy/guardrails/guardrail_endpoints.py | Replaces raw os.path.join with safe_join for category YAML lookup; ValueError is correctly converted to a 400 HTTPException. |
| litellm/proxy/prompts/prompt_endpoints.py | Adds safe_filename() on upload, but ValueError from that call falls through to the catch-all except block, returning HTTP 500 instead of 400 for invalid client-supplied filenames. |
| tests/test_litellm/proxy/common_utils/test_path_utils.py | Covers traversal, null-byte, and safe-name cases for both utilities; single-dot filename is not tested but the production code handles it. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User Request] --> B{Endpoint}
B --> C["GET /guardrails/ui/category_yaml/{category_name}"]
B --> D["POST /utils/dotprompt_json_converter"]
C --> E["os.path.join(__file__, 'categories') → categories_dir (trusted)"]
E --> F["safe_join(categories_dir, category_name + '.yaml/.json')"]
F --> G{ValueError?}
G -- Yes --> H[HTTP 400 Invalid category name]
G -- No --> I[os.path.exists check]
I -- Not found --> J[HTTP 404]
I -- Found --> K[Read & return YAML/JSON]
D --> L["file.filename.endswith('.prompt') check"]
L -- No --> M[HTTP 400]
L -- Yes --> N["safe_filename(file.filename)"]
N --> O{ValueError?}
O -- Yes --> P["Caught by except Exception → HTTP 500 (should be 400)"]
O -- No --> Q["Path(mkdtemp()) / sanitized_name"]
Q --> R[Write bytes & convert to JSON]
R --> S[Return JSON result]
Reviews (2): Last reviewed commit: "style: move import os to module level, f..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| from pathlib import Path | ||
| from typing import Any, Dict, List, Optional, cast | ||
|
|
||
| from litellm.proxy.common_utils.path_utils import safe_filename |
There was a problem hiding this comment.
from litellm.proxy.common_utils.path_utils import safe_filename is a local import placed between the stdlib block and the fastapi/pydantic third-party block. PEP 8 and the project's isort config expect the order: stdlib → third-party → local. Ruff will flag this when make lint runs.
Move this line below the pydantic import (after all third-party imports).
| """ | ||
| Safe filesystem path construction for user-controlled inputs. | ||
|
|
||
| Use safe_join() instead of os.path.join() whenever a path component | ||
| comes from user input (request parameters, uploaded filenames, etc.) | ||
| to prevent directory traversal attacks. | ||
| """ | ||
|
|
||
| import os | ||
| from pathlib import Path |
There was a problem hiding this comment.
The pre-submission checklist marks "I have Added testing in the tests/test_litellm/ directory" as completed, but the changeset contains only three source files — no test file for safe_join() or safe_filename(). CLAUDE.md designates this a hard requirement. Basic unit tests should cover at minimum:
safe_joinblocks../../traversalsafe_joinblocks null-byte injectionsafe_joinallows a valid subdirectory namesafe_filenamestrips directory components (Unix and Windows separators)safe_filenamerejects empty/./..filenames
| except ValueError: | ||
| raise HTTPException(status_code=400, detail="Invalid category name") |
There was a problem hiding this comment.
safe_join called twice when a single traversal check suffices
safe_join calls os.path.realpath twice — once per extension variant — even though only one path will ultimately be opened. The traversal check is the same for both since only the extension differs. Consider validating category_name once (e.g. with safe_filename) before constructing either path, which would make the intent clearer and halve the realpath calls.
This is a style suggestion only; the current approach is functionally correct.
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!
yuneng-berri
merged commit be1b802
into
BerriAI:litellm_yj_apr15
2 participants
Relevant issues
Fixes unsafe path construction in guardrail category endpoint and dotprompt file converter.
Pre-Submission checklist
tests/test_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 reviewType
🐛 Bug Fix
Changes
1. New shared utility:
proxy/common_utils/path_utils.pysafe_join(base_dir, *parts)— joins path components and verifies the resolved path stays within base_dir. Rejects null bytes and resolves symlinks.safe_filename(filename)— extracts the basename from a user-supplied path, normalizing both Unix and Windows separators. Rejects null bytes and unsafe names.2. Guardrail category YAML endpoint
GET /guardrails/ui/category_yaml/{category_name}now usessafe_join()instead of rawos.path.join()to construct the category file path. Returns 400 for invalid category names.3. Dotprompt file converter
POST /utils/dotprompt_json_converternow usessafe_filename()on the uploaded filename before writing to the temp directory.