Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .jules/sentinel.md
Original file line number Diff line number Diff line change
@@ -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.
## 2024-05-18 - Path traversal fix and Error leakage mitigation
**Vulnerability:** SafeStaticFiles incorrectly initialized path objects using `Path(path).lower()`, leading to `AttributeError` bypassing static file filtering. Additionally, upload exception exposed raw exception strings via `raise HTTPException(..., detail=f"Upload failed: {e}")`.
**Learning:** `Path()` objects do not have a `.lower()` method. Using `p.parts` and `p.name` on a non-existent lowercase string logic bypasses directory filtering (like blocking `00_CONFIG`). Error messages should always log detailed exceptions server-side and provide safe, generic messages to the client.
**Prevention:** Always convert the string path to lowercase before passing to `Path()` constructor, e.g. `Path(path.lower())`. Log exceptions with `logger.error` or `logger.exception` and raise generic `HTTPException` detail messages.
2 changes: 1 addition & 1 deletion src/audioformation/server/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class SafeStaticFiles(StaticFiles):

async def get_response(self, path: str, scope) -> Response:
# Normalize path for check
p = Path(path).lower()
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"
Expand Down
3 changes: 2 additions & 1 deletion src/audioformation/server/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,8 @@ async def ingest_files(
shutil.copyfileobj(file.file, buffer)
except Exception as e:
shutil.rmtree(tmp_dir, ignore_errors=True)
raise HTTPException(status_code=500, detail=f"Upload failed: {e}")
logger.error(f"Upload failed: {e}")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While using logger.error is a good step to prevent error leakage, logger.exception is preferable within an except block. It automatically includes the full stack trace in the log, which is invaluable for debugging, without needing to format the exception into the string. This provides richer diagnostic information.

Suggested change
logger.error(f"Upload failed: {e}")
logger.exception("Upload failed")

raise HTTPException(status_code=500, detail="Upload failed")

background_tasks.add_task(
_run_with_status,
Expand Down
Loading