mirror of
https://github.com/OpenHands/OpenHands.git
synced 2025-12-26 05:48:36 +08:00
Add ForbiddenError exception and proper HTTP status code handling
- Add ForbiddenError exception for permission-denied 403 errors - Update http_client.py to raise ForbiddenError instead of UnknownException for non-rate-limit 403s - Add exception handlers in app.py to return proper HTTP status codes: * 429 for RateLimitError * 403 for ForbiddenError - Update tests to use ForbiddenError instead of UnknownException This ensures: - Rate limit errors (403 with 'rate limit' message) -> HTTP 429 - Permission errors (403 without rate limit) -> HTTP 403 - Unknown errors -> HTTP 500 Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
parent
c616687b1a
commit
9212efcde2
@ -9,6 +9,7 @@ from pydantic import SecretStr
|
||||
from openhands.core.logger import openhands_logger as logger
|
||||
from openhands.integrations.service_types import (
|
||||
AuthenticationError,
|
||||
ForbiddenError,
|
||||
RateLimitError,
|
||||
RequestMethod,
|
||||
ResourceNotFoundError,
|
||||
@ -111,10 +112,11 @@ class HTTPClient(ABC):
|
||||
return RateLimitError(f'{self.provider} API rate limit exceeded')
|
||||
except Exception as json_error:
|
||||
logger.debug(f'Failed to parse 403 error response as JSON: {json_error}')
|
||||
pass # If we can't parse the response, treat as unknown error
|
||||
pass # If we can't parse the response, treat as forbidden error
|
||||
|
||||
# Not a rate limit, so it's a permission/access issue
|
||||
logger.warning(f'Forbidden error on {self.provider} API: {e}')
|
||||
return UnknownException(f'Forbidden error: {e}')
|
||||
return ForbiddenError(f'Access forbidden: {e}')
|
||||
|
||||
logger.warning(f'Status error on {self.provider} API: {e}')
|
||||
return UnknownException(f'Unknown error: {e}')
|
||||
|
||||
@ -178,6 +178,12 @@ class RateLimitError(ValueError):
|
||||
pass
|
||||
|
||||
|
||||
class ForbiddenError(ValueError):
|
||||
"""Raised when access to a resource is forbidden (permissions issue)."""
|
||||
|
||||
pass
|
||||
|
||||
|
||||
class ResourceNotFoundError(ValueError):
|
||||
"""Raised when a requested resource (file, directory, etc.) is not found."""
|
||||
|
||||
|
||||
@ -17,7 +17,11 @@ from fastapi.responses import JSONResponse
|
||||
import openhands.agenthub # noqa F401 (we import this to get the agents registered)
|
||||
from openhands.app_server import v1_router
|
||||
from openhands.app_server.config import get_app_lifespan_service
|
||||
from openhands.integrations.service_types import AuthenticationError
|
||||
from openhands.integrations.service_types import (
|
||||
AuthenticationError,
|
||||
ForbiddenError,
|
||||
RateLimitError,
|
||||
)
|
||||
from openhands.server.routes.conversation import app as conversation_api_router
|
||||
from openhands.server.routes.feedback import app as feedback_api_router
|
||||
from openhands.server.routes.files import app as files_api_router
|
||||
@ -80,6 +84,22 @@ async def authentication_error_handler(request: Request, exc: AuthenticationErro
|
||||
)
|
||||
|
||||
|
||||
@app.exception_handler(RateLimitError)
|
||||
async def rate_limit_error_handler(request: Request, exc: RateLimitError):
|
||||
return JSONResponse(
|
||||
status_code=429,
|
||||
content=str(exc),
|
||||
)
|
||||
|
||||
|
||||
@app.exception_handler(ForbiddenError)
|
||||
async def forbidden_error_handler(request: Request, exc: ForbiddenError):
|
||||
return JSONResponse(
|
||||
status_code=403,
|
||||
content=str(exc),
|
||||
)
|
||||
|
||||
|
||||
app.include_router(public_api_router)
|
||||
app.include_router(files_api_router)
|
||||
app.include_router(security_api_router)
|
||||
|
||||
@ -1,9 +1,9 @@
|
||||
"""
|
||||
Unit tests for GitHub rate limit error handling.
|
||||
Unit tests for GitHub rate limit and forbidden error handling.
|
||||
|
||||
Tests cover:
|
||||
1. Rate limit error detection (403 with rate limit message)
|
||||
2. Permission denied error handling (403 without rate limit)
|
||||
2. Permission denied error handling (403 without rate limit message)
|
||||
"""
|
||||
|
||||
from unittest.mock import AsyncMock, Mock, patch
|
||||
@ -11,7 +11,7 @@ from unittest.mock import AsyncMock, Mock, patch
|
||||
import pytest
|
||||
|
||||
from openhands.integrations.github.service.repos import GithubReposService
|
||||
from openhands.integrations.protocols.http_client import RateLimitError, UnknownException
|
||||
from openhands.integrations.service_types import ForbiddenError, RateLimitError
|
||||
from openhands.server.shared import AppMode
|
||||
|
||||
|
||||
@ -61,7 +61,7 @@ class TestRateLimitHandling:
|
||||
@pytest.mark.asyncio
|
||||
async def test_forbidden_error_non_rate_limit(self, repos_service):
|
||||
"""
|
||||
Test that 403 without rate limit message is raised as UnknownException.
|
||||
Test that 403 without rate limit message is raised as ForbiddenError.
|
||||
|
||||
Not all 403 errors are rate limits - could be permission denied, etc.
|
||||
"""
|
||||
@ -74,9 +74,9 @@ class TestRateLimitHandling:
|
||||
}
|
||||
|
||||
with patch.object(
|
||||
repos_service, '_make_request', side_effect=UnknownException(mock_response)
|
||||
repos_service, '_make_request', side_effect=ForbiddenError(mock_response)
|
||||
):
|
||||
with pytest.raises(UnknownException) as exc_info:
|
||||
with pytest.raises(ForbiddenError) as exc_info:
|
||||
await repos_service.search_repositories(
|
||||
query='test',
|
||||
per_page=100,
|
||||
@ -86,6 +86,6 @@ class TestRateLimitHandling:
|
||||
app_mode=AppMode.SAAS,
|
||||
)
|
||||
|
||||
# Verify it's UnknownException, not RateLimitError
|
||||
assert isinstance(exc_info.value, UnknownException)
|
||||
# Verify it's ForbiddenError, not RateLimitError
|
||||
assert isinstance(exc_info.value, ForbiddenError)
|
||||
assert not isinstance(exc_info.value, RateLimitError)
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user