fix(security): block path traversal in skill_view file_path (fixes #220)
skill_view accepted arbitrary file_path values like '../../.env' and would read files outside the skill directory, exposing API keys and other sensitive data. Added two layers of defense: 1. Reject paths with '..' components (fast, catches obvious traversal) 2. resolve() containment check with trailing '/' to prevent prefix collisions (catches symlinks and edge cases) Fix approach from PR #242 (@Bartok9). Vulnerability reported by @Farukest (#220, PR #221). Tests rewritten to properly mock SKILLS_DIR. Closes #220
This commit is contained in:
parent
25c65bc99e
commit
1cb2311bad
2 changed files with 109 additions and 0 deletions
|
|
@ -443,7 +443,33 @@ def skill_view(name: str, file_path: str = None, task_id: str = None) -> str:
|
|||
|
||||
# If a specific file path is requested, read that instead
|
||||
if file_path and skill_dir:
|
||||
# Security: Prevent path traversal attacks
|
||||
normalized_path = Path(file_path)
|
||||
if ".." in normalized_path.parts:
|
||||
return json.dumps({
|
||||
"success": False,
|
||||
"error": "Path traversal ('..') is not allowed.",
|
||||
"hint": "Use a relative path within the skill directory"
|
||||
}, ensure_ascii=False)
|
||||
|
||||
target_file = skill_dir / file_path
|
||||
|
||||
# Security: Verify resolved path is still within skill directory
|
||||
try:
|
||||
resolved = target_file.resolve()
|
||||
skill_dir_resolved = skill_dir.resolve()
|
||||
if not str(resolved).startswith(str(skill_dir_resolved) + "/") and resolved != skill_dir_resolved:
|
||||
return json.dumps({
|
||||
"success": False,
|
||||
"error": "Path escapes skill directory boundary.",
|
||||
"hint": "Use a relative path within the skill directory"
|
||||
}, ensure_ascii=False)
|
||||
except (OSError, ValueError):
|
||||
return json.dumps({
|
||||
"success": False,
|
||||
"error": f"Invalid file path: '{file_path}'",
|
||||
"hint": "Use a valid relative path within the skill directory"
|
||||
}, ensure_ascii=False)
|
||||
if not target_file.exists():
|
||||
# List available files in the skill directory, organized by type
|
||||
available_files = {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue