🛡️ Sentinel: [CRITICAL] Fix path traversal in file_browser.py

.jules/sentinel.md

@@ -0,0 +1,5 @@
+## 2024-05-30 - Path Traversal via str.startswith
+
+**Vulnerability:** Path traversal in `helpers.file_browser` where paths were validated using `str(full_path).startswith(str(self.base_dir))`.
+**Learning:** Checking path containment with string operations is insecure because `/app-secrets/secret.txt` starts with `/app`, but `/app-secrets` is not inside `/app`.
+**Prevention:** Always validate path containment using `is_relative_to()` on `pathlib.Path` objects, or rely on `helpers.files.is_in_base_dir()`.

helpers/file_browser.py

@@ -42,7 +42,7 @@ def save_file_b64(self, current_path: str, filename: str, base64_content: str):
         try:
             # Resolve the target directory path
             target_file = (self.base_dir / current_path / filename).resolve()
-            if not str(target_file).startswith(str(self.base_dir)):
+            if not target_file.is_relative_to(self.base_dir):
                 raise ValueError("Invalid target directory")
 
             os.makedirs(target_file.parent, exist_ok=True)
@@ -62,7 +62,7 @@ def save_files(self, files: List, current_path: str = "") -> Tuple[List[str], Li
         try:
             # Resolve the target directory path
             target_dir = (self.base_dir / current_path).resolve()
-            if not str(target_dir).startswith(str(self.base_dir)):
+            if not target_dir.is_relative_to(self.base_dir):
                 raise ValueError("Invalid target directory")
 
             os.makedirs(target_dir, exist_ok=True)
@@ -94,7 +94,7 @@ def delete_file(self, file_path: str) -> bool:
         try:
             # Resolve the full path while preventing directory traversal
             full_path = (self.base_dir / file_path).resolve()
-            if not str(full_path).startswith(str(self.base_dir)):
+            if not full_path.is_relative_to(self.base_dir):
                 raise ValueError("Invalid path")
 
             if os.path.exists(full_path):
@@ -118,13 +118,13 @@ def rename_item(self, file_path: str, new_name: str) -> bool:
                 raise ValueError("New name cannot include path separators")
 
             full_path = (self.base_dir / file_path).resolve()
-            if not str(full_path).startswith(str(self.base_dir)):
+            if not full_path.is_relative_to(self.base_dir):
                 raise ValueError("Invalid path")
             if not full_path.exists():
                 raise FileNotFoundError("File or folder not found")
 
             new_path = full_path.with_name(new_name)
-            if not str(new_path).startswith(str(self.base_dir)):
+            if not new_path.is_relative_to(self.base_dir):
                 raise ValueError("Invalid target path")
             if full_path == new_path:
                 return True
@@ -145,11 +145,11 @@ def create_folder(self, parent_path: str, folder_name: str) -> bool:
                 raise ValueError("Folder name cannot include path separators")
 
             parent_full = (self.base_dir / parent_path).resolve()
-            if not str(parent_full).startswith(str(self.base_dir)):
+            if not parent_full.is_relative_to(self.base_dir):
                 raise ValueError("Invalid parent path")
 
             target_dir = (parent_full / folder_name).resolve()
-            if not str(target_dir).startswith(str(self.base_dir)):
+            if not target_dir.is_relative_to(self.base_dir):
                 raise ValueError("Invalid target path")
             if target_dir.exists():
                 raise FileExistsError("Folder already exists")
@@ -169,7 +169,7 @@ def save_text_file(self, file_path: str, content: str) -> bool:
                 raise ValueError("File exceeds 1 MB and cannot be edited")
 
             full_path = (self.base_dir / file_path).resolve()
-            if not str(full_path).startswith(str(self.base_dir)):
+            if not full_path.is_relative_to(self.base_dir):
                 raise ValueError("Invalid path")
             if full_path.exists() and full_path.is_dir():
                 raise ValueError("Target is a directory")
@@ -307,7 +307,7 @@ def get_files(self, current_path: str = "") -> Dict:
         try:
             # Resolve the full path while preventing directory traversal
             full_path = (self.base_dir / current_path).resolve()
-            if not str(full_path).startswith(str(self.base_dir)):
+            if not full_path.is_relative_to(self.base_dir):
                 raise ValueError("Invalid path")
 
             # Use ls command instead of os.scandir for better error handling