fix(url-upload): handle unsafe redirects as client errors
This commit is contained in:
+13
-17
@@ -28,6 +28,10 @@ logger = logging.getLogger(__name__)
|
|||||||
router = APIRouter()
|
router = APIRouter()
|
||||||
|
|
||||||
|
|
||||||
|
class UnsafeRedirectError(httpx.RequestError):
|
||||||
|
"""Raised when a redirect target fails URL safety checks."""
|
||||||
|
|
||||||
|
|
||||||
class URLUploadRequest(BaseModel):
|
class URLUploadRequest(BaseModel):
|
||||||
"""Request model for URL-based file upload"""
|
"""Request model for URL-based file upload"""
|
||||||
|
|
||||||
@@ -120,9 +124,10 @@ async def verify_redirect(response: httpx.Response) -> None:
|
|||||||
try:
|
try:
|
||||||
validate_url_safety(new_url)
|
validate_url_safety(new_url)
|
||||||
except HTTPException as e:
|
except HTTPException as e:
|
||||||
# Map the validation error to an httpx exception so it can be handled
|
raise UnsafeRedirectError(
|
||||||
# properly by the caller, avoiding raw HTTPExceptions escaping the client scope
|
f"Redirect to unsafe URL blocked: {e.detail}",
|
||||||
raise httpx.RequestError(f"Redirect to unsafe URL blocked: {e.detail}", request=response.request) from e
|
request=response.request,
|
||||||
|
) from e
|
||||||
|
|
||||||
|
|
||||||
@router.post("/process-url")
|
@router.post("/process-url")
|
||||||
@@ -170,19 +175,6 @@ async def process_url(
|
|||||||
if not safe_filename:
|
if not safe_filename:
|
||||||
safe_filename = "download"
|
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
|
# Download file with security measures
|
||||||
# Initialize target_path to None to prevent UnboundLocalError in exception handlers
|
# Initialize target_path to None to prevent UnboundLocalError in exception handlers
|
||||||
# that may execute before target_path is assigned during error cases
|
# that may execute before target_path is assigned during error cases
|
||||||
@@ -194,7 +186,7 @@ async def process_url(
|
|||||||
async with httpx.AsyncClient(
|
async with httpx.AsyncClient(
|
||||||
timeout=settings.http_request_timeout,
|
timeout=settings.http_request_timeout,
|
||||||
follow_redirects=True,
|
follow_redirects=True,
|
||||||
event_hooks={"response": [validate_redirect, verify_redirect]},
|
event_hooks={"response": [verify_redirect]},
|
||||||
headers={
|
headers={
|
||||||
"User-Agent": "DocuElevate/1.0", # Identify ourselves
|
"User-Agent": "DocuElevate/1.0", # Identify ourselves
|
||||||
},
|
},
|
||||||
@@ -284,6 +276,10 @@ async def process_url(
|
|||||||
logger.error(f"HTTP error while downloading file from URL: {url} - {str(e)}")
|
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)}")
|
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:
|
except httpx.RequestError as e:
|
||||||
logger.error(f"Error downloading file from URL: {url} - {str(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)}")
|
raise HTTPException(status_code=500, detail=f"Failed to download file: {str(e)}")
|
||||||
|
|||||||
@@ -465,6 +465,19 @@ class TestURLUploadEndpoint:
|
|||||||
data = response.json()
|
data = response.json()
|
||||||
assert "Failed to download file" in data["detail"]
|
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")
|
@patch("app.api.url_upload.httpx.AsyncClient.stream")
|
||||||
def test_process_url_oserror_during_save(self, mock_stream, client, tmp_path, monkeypatch):
|
def test_process_url_oserror_during_save(self, mock_stream, client, tmp_path, monkeypatch):
|
||||||
"""Test handling of OSError when saving file"""
|
"""Test handling of OSError when saving file"""
|
||||||
|
|||||||
Reference in New Issue
Block a user