diff --git a/.jules/sentinel.md b/.jules/sentinel.md index c1caaeb..e46221e 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -6,3 +6,13 @@ **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-23 - Unhandled Exceptions in Path Handling +**Vulnerability:** Core path validation routines (`validate_path_within`) and security middleware (`SafeStaticFiles`) failed to account for `None` inputs or unexpected types, leading to unhandled exceptions (`AttributeError`, `TypeError`). If an unhandled exception occurred during security validation, it could result in denial of service (500 errors) or, depending on upstream error handling, failing open to expose sensitive files. +**Learning:** Security controls that rely on type-specific methods (like `pathlib.Path.resolve` or `str.lower()`) must strictly validate input types (e.g., `is not None`) and wrap operations in comprehensive `try...except` blocks that catch `TypeError` and `AttributeError` in addition to filesystem exceptions. +**Prevention:** When building security gates, always assume malformed or null inputs. Explicitly check for `None` and ensure the exception handler catches all potential casting/method errors to fail closed securely. + +## 2025-02-23 - Catching Unhandled Exceptions in Path Validation +**Vulnerability:** The core path validation routine `validate_path_within` could fail open if an upstream component supplied a malformed input (like `None`), which caused unexpected unhandled exceptions (`AttributeError`, `TypeError`) before the default `except` block. +**Learning:** Type enforcement failures should be caught gracefully in security boundaries. When interacting with built-in path libraries, include `TypeError` and `AttributeError` in the exception block. Adding an arbitrary try/catch on safe built-ins (like `str.lower()`) constitutes "security theater". +**Prevention:** Catch specific exceptions (`TypeError`, `AttributeError`) within the target library calls rather than wrapping large chunks of business logic in a generic `Exception` catch, which might accidentally mask explicitly raised HTTP errors like 403 Forbidden. diff --git a/src/audioformation/server/app.py b/src/audioformation/server/app.py index 54243ef..50af0e4 100644 --- a/src/audioformation/server/app.py +++ b/src/audioformation/server/app.py @@ -23,8 +23,12 @@ class SafeStaticFiles(StaticFiles): """ async def get_response(self, path: str, scope) -> Response: + if path is None: + raise HTTPException(status_code=400, detail="Invalid path") + # 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" diff --git a/src/audioformation/utils/security.py b/src/audioformation/utils/security.py index abcdc30..6a437cd 100644 --- a/src/audioformation/utils/security.py +++ b/src/audioformation/utils/security.py @@ -81,7 +81,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, AttributeError, OSError): return False diff --git a/tests/test_security.py b/tests/test_security.py index 2fc9205..e43dae0 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -127,6 +127,12 @@ def test_symlink_traversal_rejected(self, tmp_path: Path) -> None: # because it resolves outside the root. assert validate_path_within(traversal_path, sandbox) is False + def test_validate_path_within_none(self, tmp_path: Path) -> None: + assert validate_path_within(None, tmp_path) is False + + def test_validate_path_within_none_root(self, tmp_path: Path) -> None: + assert validate_path_within(tmp_path, None) is False + class TestRedactApiKeys: """Tests for API key redaction in logging."""