Compare commits
8 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| afff66a089 | |||
| aa34516105 | |||
| 7a8dc3f456 | |||
| e1e6b5213c | |||
| 40e22a06ef | |||
| 725018a6f5 | |||
| 6108a0fc2b | |||
| 54f2b23075 |
@@ -28,3 +28,7 @@
|
||||
**Vulnerability:** The `/process-url` endpoint used `httpx.AsyncClient(follow_redirects=True)` after validating the initial user-provided URL against SSRF protections. However, it did not validate the target URLs of any subsequent HTTP redirects, allowing an attacker to provide a safe URL that redirects to an internal/private IP, bypassing the security check.
|
||||
**Learning:** Initial URL validation is insufficient when the HTTP client is configured to follow redirects automatically. The client must be explicitly configured to validate every redirect target.
|
||||
**Prevention:** When using `httpx.AsyncClient(follow_redirects=True)` for user-provided URLs, always implement a redirect validator hook function (e.g., using `event_hooks={'response': [validate_redirect]}`) that resolves the `Location` header and passes it through the same SSRF validation logic before the redirect is followed.
|
||||
## 2026-03-27 - SyntaxError: keyword argument repeated in httpx event hooks
|
||||
**Vulnerability:** The `/process-url` endpoint in `app/api/url_upload.py` initialized `httpx.AsyncClient` with the `event_hooks` keyword argument twice. This caused a Python SyntaxError, effectively crashing the API endpoint and preventing any execution.
|
||||
**Learning:** Python does not allow duplicate keyword arguments. In scenarios where multiple hooks (like local and module-level SSRF interceptors) must be provided to a client, they must be merged into a single list value.
|
||||
**Prevention:** Combine multiple callables for the same event key into a single list, e.g., `event_hooks={"response": [hook1, hook2]}`.
|
||||
|
||||
+1
-1
@@ -1 +1 @@
|
||||
2026-05-17T12:40:20Z
|
||||
2026-04-07T09:34:53Z
|
||||
|
||||
@@ -10,19 +10,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||
|
||||
<!-- version list -->
|
||||
|
||||
## v0.172.10 (2026-05-17)
|
||||
|
||||
### Bug Fixes
|
||||
|
||||
- **url-upload**: Handle unsafe redirects as client errors
|
||||
([`871f788`](https://github.com/christianlouis/DocuElevate/commit/871f788f0bd782ba8ad3a7d70e5cd4ccd24f749b))
|
||||
|
||||
### Documentation
|
||||
|
||||
- **changelog**: Update changelog [skip ci]
|
||||
([`58b14ae`](https://github.com/christianlouis/DocuElevate/commit/58b14ae769b85e25290126256de936743609af06))
|
||||
|
||||
|
||||
## Unreleased
|
||||
|
||||
|
||||
|
||||
+6
-6
@@ -1,10 +1,10 @@
|
||||
DocuElevate Build Information
|
||||
==============================
|
||||
Version: 0.172.10
|
||||
Build Date: 2026-05-17T12:40:20Z
|
||||
Git Commit: 62d4ca6367a6c8a2e7909305fec56c5b2c24e312
|
||||
Git Short SHA: 62d4ca6
|
||||
Version: 0.172.9
|
||||
Build Date: 2026-04-07T09:34:53Z
|
||||
Git Commit: 3bd8a52ea201b33d6071c9b3a7fdace582e65fd5
|
||||
Git Short SHA: 3bd8a52
|
||||
Git Branch: main
|
||||
Commit Date: 2026-05-17T14:39:59+02:00
|
||||
Build Timestamp: 2026-05-17T12:40:20Z
|
||||
Commit Date: 2026-04-07T11:34:28+02:00
|
||||
Build Timestamp: 2026-04-07T09:34:53Z
|
||||
==============================
|
||||
|
||||
+17
-13
@@ -28,10 +28,6 @@ logger = logging.getLogger(__name__)
|
||||
router = APIRouter()
|
||||
|
||||
|
||||
class UnsafeRedirectError(httpx.RequestError):
|
||||
"""Raised when a redirect target fails URL safety checks."""
|
||||
|
||||
|
||||
class URLUploadRequest(BaseModel):
|
||||
"""Request model for URL-based file upload"""
|
||||
|
||||
@@ -124,10 +120,9 @@ async def verify_redirect(response: httpx.Response) -> None:
|
||||
try:
|
||||
validate_url_safety(new_url)
|
||||
except HTTPException as e:
|
||||
raise UnsafeRedirectError(
|
||||
f"Redirect to unsafe URL blocked: {e.detail}",
|
||||
request=response.request,
|
||||
) from e
|
||||
# Map the validation error to an httpx exception so it can be handled
|
||||
# properly by the caller, avoiding raw HTTPExceptions escaping the client scope
|
||||
raise httpx.RequestError(f"Redirect to unsafe URL blocked: {e.detail}", request=response.request) from e
|
||||
|
||||
|
||||
@router.post("/process-url")
|
||||
@@ -175,6 +170,19 @@ async def process_url(
|
||||
if not safe_filename:
|
||||
safe_filename = "download"
|
||||
|
||||
# Hook to validate redirects and prevent SSRF
|
||||
async def validate_redirect(response: httpx.Response):
|
||||
if response.is_redirect:
|
||||
location = response.headers.get("Location")
|
||||
if location:
|
||||
# Resolve relative URLs
|
||||
next_url = urllib.parse.urljoin(str(response.url), location)
|
||||
try:
|
||||
validate_url_safety(next_url)
|
||||
except HTTPException as e:
|
||||
# Reraise as a RequestError so httpx aborts the request
|
||||
raise httpx.RequestError(f"Unsafe redirect target: {e.detail}", request=response.request)
|
||||
|
||||
# Download file with security measures
|
||||
# Initialize target_path to None to prevent UnboundLocalError in exception handlers
|
||||
# that may execute before target_path is assigned during error cases
|
||||
@@ -186,7 +194,7 @@ async def process_url(
|
||||
async with httpx.AsyncClient(
|
||||
timeout=settings.http_request_timeout,
|
||||
follow_redirects=True,
|
||||
event_hooks={"response": [verify_redirect]},
|
||||
event_hooks={"response": [validate_redirect, verify_redirect]},
|
||||
headers={
|
||||
"User-Agent": "DocuElevate/1.0", # Identify ourselves
|
||||
},
|
||||
@@ -276,10 +284,6 @@ async def process_url(
|
||||
logger.error(f"HTTP error while downloading file from URL: {url} - {str(e)}")
|
||||
raise HTTPException(status_code=e.response.status_code, detail=f"HTTP error: {str(e)}")
|
||||
|
||||
except UnsafeRedirectError as e:
|
||||
logger.warning(f"Unsafe redirect blocked while downloading file from URL: {url} - {str(e)}")
|
||||
raise HTTPException(status_code=400, detail=str(e))
|
||||
|
||||
except httpx.RequestError as e:
|
||||
logger.error(f"Error downloading file from URL: {url} - {str(e)}")
|
||||
raise HTTPException(status_code=500, detail=f"Failed to download file: {str(e)}")
|
||||
|
||||
+23
-13
@@ -465,19 +465,6 @@ class TestURLUploadEndpoint:
|
||||
data = response.json()
|
||||
assert "Failed to download file" in data["detail"]
|
||||
|
||||
@patch("app.api.url_upload.httpx.AsyncClient.stream")
|
||||
def test_process_url_unsafe_redirect_returns_400(self, mock_stream, client):
|
||||
"""Test unsafe redirects are reported as a client error instead of HTTP 500."""
|
||||
from app.api.url_upload import UnsafeRedirectError
|
||||
|
||||
mock_stream.side_effect = UnsafeRedirectError("Redirect to unsafe URL blocked: Unsafe URL")
|
||||
|
||||
response = client.post("/api/process-url", json={"url": "https://example.com/file.pdf"})
|
||||
|
||||
assert response.status_code == 400
|
||||
data = response.json()
|
||||
assert "Redirect to unsafe URL blocked" in data["detail"]
|
||||
|
||||
@patch("app.api.url_upload.httpx.AsyncClient.stream")
|
||||
def test_process_url_oserror_during_save(self, mock_stream, client, tmp_path, monkeypatch):
|
||||
"""Test handling of OSError when saving file"""
|
||||
@@ -925,6 +912,29 @@ class TestURLUploadCoverageGaps:
|
||||
|
||||
assert "Redirect to unsafe URL blocked" in str(exc_info.value)
|
||||
|
||||
@patch("app.api.url_upload.validate_url_safety")
|
||||
@patch("app.api.url_upload.httpx.AsyncClient.stream")
|
||||
def test_process_url_validate_redirect_hook_blocks_unsafe_url(self, mock_stream, mock_validate, client):
|
||||
"""Test that the local validate_redirect hook successfully aborts the request when redirect is unsafe"""
|
||||
import httpx
|
||||
|
||||
# The local validate_redirect hook intercepts 301/302 and throws an httpx.RequestError
|
||||
# Here we mock the behavior of that hook executing during the stream context
|
||||
def side_effect(*args, **kwargs):
|
||||
# Raise a simulated RequestError caused by validate_redirect
|
||||
raise httpx.RequestError(
|
||||
"Unsafe redirect target: Access to private IP addresses is not allowed",
|
||||
request=httpx.Request("GET", "http://example.com"),
|
||||
)
|
||||
|
||||
mock_stream.side_effect = side_effect
|
||||
|
||||
response = client.post("/api/process-url", json={"url": "http://example.com"})
|
||||
|
||||
# Our exception handler in process_url converts RequestError to a 500 HTTPException
|
||||
assert response.status_code == 500
|
||||
assert "Unsafe redirect target" in response.json()["detail"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_verify_redirect_ignores_non_redirects(self):
|
||||
"""Test verify_redirect ignores 200 OK responses"""
|
||||
|
||||
Reference in New Issue
Block a user