From c8233275a257c8005b810a0d01ec95c2d4485353 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Fri, 15 May 2026 21:50:14 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[MEDIUM]=20?= =?UTF-8?q?Enhance=20defensive=20coding=20in=20routes=20and=20path=20valid?= =?UTF-8?q?ation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: socialawy <24765060+socialawy@users.noreply.github.com> --- .jules/sentinel.md | 5 +++ src/audioformation/server/routes.py | 52 ++++++++++++++++++++-------- src/audioformation/utils/security.py | 5 ++- 3 files changed, 47 insertions(+), 15 deletions(-) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index c1caaeb..cd09d99 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -6,3 +6,8 @@ **Vulnerability:** The custom `SafeStaticFiles` middleware in `src/audioformation/server/app.py` intended to block access to sensitive directories (like `00_CONFIG` and `.git`). However, it used `p = Path(path).lower()`, which raises an `AttributeError` because `pathlib.Path` objects lack a `.lower()` method. This effectively broke static file serving entirely (causing 500 errors) and represented a malformed security check. If such errors were ever 'swallowed' without raising an HTTP exception, it could result in 'failing open' and allowing access to sensitive files. **Learning:** Security checks that rely on path manipulation or normalization must be carefully tested for runtime exceptions. An unhandled exception in a security gate can either block legitimate traffic (Denial of Service) or, if caught improperly elsewhere, fail open. Always normalize the string representation of paths before converting them to `Path` objects. **Prevention:** Thoroughly test security middleware endpoints for both valid and invalid access attempts. Ensure that path string normalizations like `.lower()` are applied directly to the string before instantiating `Path(str(path).lower())`. + +## 2025-02-24 - Unhandled Exceptions Leaking Information and DoS +**Vulnerability:** Several endpoints in `routes.py` and the `validate_path_within` helper used broad `try...except Exception` blocks that caught standard library exceptions (`ValueError`, `TypeError`, `OSError`) and returned an HTTP 500 without logging the exception via `logger.exception(..., exc_info=True)`. Additionally, the `validate_path_within` helper didn't handle null inputs explicitly. +**Learning:** Broad exception handling that returns HTTP errors without logging masks the underlying system failures (like disk full or invalid paths), complicating debugging. Path utilities must handle `None` gracefully and limit caught exceptions to expected standard library errors. +**Prevention:** Always log caught exceptions using `logger.error(..., exc_info=True)` before returning a generic 500 error to the user, and ensure utility functions handle `NoneType` to fail safely and gracefully without crashing the server. diff --git a/src/audioformation/server/routes.py b/src/audioformation/server/routes.py index 34c9bec..f2e1969 100644 --- a/src/audioformation/server/routes.py +++ b/src/audioformation/server/routes.py @@ -208,15 +208,25 @@ async def upload_file(project_id: str, category: str, file: UploadFile = File(.. target_dir = project_path / "05_MUSIC" / "generated" else: raise HTTPException(status_code=400, detail="Invalid category") - target_dir.mkdir(parents=True, exist_ok=True) - # Secure filename handling - safe_name = os.path.basename(file.filename) - # Additional strict regex guard - if not re.fullmatch(r"^[A-Za-z0-9_.-]+$", safe_name): - raise HTTPException(status_code=400, detail="Invalid filename characters") + try: + target_dir.mkdir(parents=True, exist_ok=True) + except Exception as e: + logger.error(f"Failed to create upload directory: {e}", exc_info=True) + raise HTTPException(status_code=500, detail="Upload failed") - dest = target_dir / safe_name + try: + # Secure filename handling + safe_name = os.path.basename(file.filename) + # Additional strict regex guard + if not re.fullmatch(r"^[A-Za-z0-9_.-]+$", safe_name): + raise HTTPException(status_code=400, detail="Invalid filename characters") + + dest = target_dir / safe_name + except HTTPException: + raise + except Exception: + raise HTTPException(status_code=400, detail="Invalid path") # Check bounds using fixed validator if not validate_path_within(dest, target_dir): @@ -229,7 +239,8 @@ async def upload_file(project_id: str, category: str, file: UploadFile = File(.. "path": str(dest.relative_to(project_path)).replace("\\", "/"), "filename": safe_name, } - except Exception: + except Exception as e: + logger.error(f"Failed to create upload directory: {e}", exc_info=True) raise HTTPException(status_code=500, detail="Upload failed") @@ -253,21 +264,34 @@ async def preview_voice(project_id: str, request: PreviewRequest): if request.reference_audio: # CODEQL FIX: Validate reference_audio is strictly within project # We do NOT call .resolve() here as it's a sink for unvalidated user input. - possible_ref = project_path / request.reference_audio + try: + possible_ref = project_path / request.reference_audio + except Exception: + raise HTTPException(status_code=400, detail="Invalid reference audio path") + if not validate_path_within(possible_ref, project_path): raise HTTPException(status_code=400, detail="Invalid reference audio path") # Now it is safe to resolve if we need the absolute path - ref_path = possible_ref.resolve() + try: + ref_path = possible_ref.resolve() + except Exception: + raise HTTPException(status_code=400, detail="Invalid reference audio path") + if not ref_path.exists(): raise HTTPException(status_code=400, detail="Reference audio not found") # Create temp file for output - with tempfile.NamedTemporaryFile(suffix=".wav", delete=False) as tmp: - output_path = Path(tmp.name) + try: + with tempfile.NamedTemporaryFile(suffix=".wav", delete=False) as tmp: + output_path = Path(tmp.name) + + # Validate temp path is within system temp directory + temp_root = Path(tempfile.gettempdir()) + except Exception as e: + logger.error(f"Failed to create temp path: {e}", exc_info=True) + raise HTTPException(status_code=500, detail="Invalid temp path") - # Validate temp path is within system temp directory - temp_root = Path(tempfile.gettempdir()) if not validate_path_within(output_path, temp_root): raise HTTPException(status_code=500, detail="Invalid temp path") diff --git a/src/audioformation/utils/security.py b/src/audioformation/utils/security.py index abcdc30..96b004a 100644 --- a/src/audioformation/utils/security.py +++ b/src/audioformation/utils/security.py @@ -67,6 +67,9 @@ def validate_path_within(path: Path, root: Path) -> bool: Ensure `path` is within `root`, resolving all symlinks. This prevents path traversal and symlink bypass attacks. """ + if path is None or root is None: + return False + try: # Resolve to absolute paths first abs_path = os.path.abspath(str(path)) @@ -81,7 +84,7 @@ def validate_path_within(path: Path, root: Path) -> bool: return resolved_path.is_relative_to(resolved_root) return False - except (ValueError, RuntimeError, OSError): + except (TypeError, ValueError, RuntimeError, OSError, AttributeError): return False