🛡️ Sentinel: [CRITICAL] Fix path traversal vulnerabilities and handle exceptions securely

.jules/sentinel.md

@@ -1,4 +1,8 @@
 ## 2025-02-21 - Path Traversal in Mix Endpoint API Parameter
 **Vulnerability:** The `/projects/{project_id}/mix` API endpoint in `src/audioformation/server/routes.py` accepted a `music` parameter (meant to specify a filename within the `05_MUSIC/generated` directory) but directly passed it to `mix_project` without sanitization. This allowed directory traversal payloads like `../../../etc/passwd` to be used for background music resolution.
 **Learning:** Even internal API inputs that map strictly to filenames inside an expected directory must be sanitized. A simple check for file existence (`if not bg_music_path.exists():`) is insufficient as it confirms existence but allows looking outside the bounded directory.
-**Prevention:** Always use established sanitization helpers (like `sanitize_filename`) or bound checks (like `validate_path_within`) for any user-supplied string that forms part of a filesystem path. Ensure bypass parameters like `FORCE_NO_MUSIC` are handled before and mutually exclusively from sanitization.
+**Prevention:** Always use established sanitization helpers (like `sanitize_filename`) or bound checks (like `validate_path_within`) for any user-supplied string that forms part of a filesystem path. Ensure bypass parameters like `FORCE_NO_MUSIC` are handled before and mutually exclusively from sanitization.
+## 2025-02-21 - Path Validation and Traversal Prevention Enhancements
+**Vulnerability:** The custom `SafeStaticFiles` middleware crashed with an `AttributeError` when a path (e.g. `NoneType`) was improperly passed since `Path(path).lower()` was used, and the system used `os.path.abspath` for path bound checks, creating a risk of symlink bypasses.
+**Learning:** `Path` object construction does not perform lowercase conversions like strings. Further, path string manipulators like `os.path.abspath` are insecure against advanced traversal via symlink resolution. Exceptions thrown during path manipulation must fail closed explicitly via a try...except returning an `HTTPException`, to not expose internal errors and avoid the service from breaking or failing open.
+**Prevention:** Convert path variables to explicitly stringified representations `Path(str(path).lower())` before doing manipulations. Always replace string based absolute path checks with `Path.resolve().is_relative_to()` to handle resolution robustly against symlink abuse and add fail-closed `try..except` handlers for any path manipulators that can raise generic exceptions.

src/audioformation/server/app.py

@@ -23,12 +23,17 @@ class SafeStaticFiles(StaticFiles):
     """
 
     async def get_response(self, path: str, scope) -> Response:
-        # Normalize path for check
-        p = Path(path).lower()
-        if "00_config" in p.parts or p.name.startswith(".env") or ".git" in p.parts:
-            raise HTTPException(
-                status_code=403, detail="Access denied to sensitive resource"
-            )
+        try:
+            # Normalize path for check
+            p = Path(str(path).lower())
+            if "00_config" in p.parts or p.name.startswith(".env") or ".git" in p.parts:
+                raise HTTPException(
+                    status_code=403, detail="Access denied to sensitive resource"
+                )
+        except HTTPException:
+            raise
+        except Exception:
+            raise HTTPException(status_code=400, detail="Invalid path")
 
         return await super().get_response(path, scope)
 

src/audioformation/utils/security.py

@@ -68,20 +68,10 @@
     This prevents path traversal and symlink bypass attacks.
     """
     try:
-        # Resolve to absolute paths first
-        abs_path = os.path.abspath(str(path))
-        abs_root = os.path.abspath(str(root))
-
-        # On Windows, abspath can have different casing for the drive letter.
-        # We normalize to lowercase for the preliminary string check.
-        if abs_path.lower().startswith(abs_root.lower()):
-            # String check passed, now do the rigorous resolution check
-            resolved_root = root.resolve()
-            resolved_path = path.resolve()
-            return resolved_path.is_relative_to(resolved_root)
-
-        return False
-    except (ValueError, RuntimeError, OSError):
+        resolved_root = root.resolve()
+        resolved_path = path.resolve()
+        return resolved_path.is_relative_to(resolved_root)
+    except (TypeError, ValueError, RuntimeError, AttributeError, OSError):
         return False