Skip to content

Commit

Permalink
fix: no proper handling of JSON server errors (#685)
Browse files Browse the repository at this point in the history
On server errors with JSON responses, the CLI was using response.text instead of response.json() to get the detail, causing string formatting conflicts. Now, it properly parses JSON responses before error handling.
  • Loading branch information
yeisonvargasf authored Feb 20, 2025
1 parent 0a99f94 commit 2474d10
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 11 deletions.
34 changes: 27 additions & 7 deletions safety/auth/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,27 @@ def is_email_verified(info: Dict[str, Any]) -> Optional[bool]:
return True


def extract_detail(response: requests.Response) -> Optional[str]:
"""
Extract the reason from an HTTP response.
Args:
response (requests.Response): The response.
Returns:
Optional[str]: The reason.
"""
detail = None

try:
detail = response.json().get("detail")
except Exception:
LOG.debug("Failed to extract detail from response: %s",
response.status_code)

return detail


def parse_response(func: Callable) -> Callable:
"""
Decorator to parse the response from an HTTP request.
Expand Down Expand Up @@ -105,12 +126,8 @@ def wrapper(*args, **kwargs):
# TODO: Handle content as JSON and fallback to text for all responses

if r.status_code == 403:
reason = None
try:
reason = r.json().get("detail")
except Exception:
LOG.debug("Failed to parse 403 response: %s", r.text)

reason = extract_detail(response=r)

raise InvalidCredentialError(
credential="Failed authentication.", reason=reason
)
Expand All @@ -130,7 +147,10 @@ def wrapper(*args, **kwargs):
raise SafetyError(message=reason, error_code=error_code)

if r.status_code >= 500:
raise ServerError(reason=f"{r.reason} - {r.text}")
reason = extract_detail(response=r)
LOG.debug("ServerError %s -> Response returned: %s",
r.status_code, r.text)
raise ServerError(reason=reason)

data = None

Expand Down
1 change: 0 additions & 1 deletion safety/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,6 @@ class ServerError(DatabaseFetchError):
"""
def __init__(self, reason: Optional[str] = None,
message: str = "Sorry, something went wrong.\n"
"Safety CLI cannot connect to the server.\n"
"Our engineers are working quickly to resolve the issue."):
info = f" Reason: {reason}"
self.message = message + (info if reason else "")
Expand Down
2 changes: 1 addition & 1 deletion safety/scan/render.py
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ def print_wait_project_verification(console: Console, project_id: str, closure:
LOG.exception(f'Unable to verify the project, reason: {e}')
reason = "We are currently unable to verify the project, " \
"and it is necessary to link the scan to a specific " \
f"project. Reason: {e}"
f"project. \n\nAdditional Information: \n{e}"
raise SafetyException(message=reason)

if not status:
Expand Down
27 changes: 25 additions & 2 deletions tests/auth/test_auth_utils.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import unittest
from unittest.mock import MagicMock, Mock, patch, call
from safety.auth.utils import initialize

from safety.auth.utils import initialize, extract_detail, FeatureType, \
str_to_bool, get_config_setting, save_flags_config
from safety.errors import InvalidCredentialError
from safety.auth.utils import FeatureType, str_to_bool, get_config_setting, save_flags_config


class TestUtils(unittest.TestCase):

Expand Down Expand Up @@ -137,3 +139,24 @@ def test_initialize_with_server_response(self,

# Verify number of calls matches number of features
self.assertEqual(mock_setattr.call_count, len(FeatureType))

def test_extract_detail(self):
# Test valid JSON with detail
response = Mock()
response.json.return_value = {"detail": "Error message"}
detail = extract_detail(response)
self.assertEqual(detail, "Error message")

# Test valid JSON without detail
response.json.return_value = {"message": "Something else"}
detail = extract_detail(response)
self.assertIsNone(detail)

# Test invalid JSON
response.json.side_effect = ValueError()
detail = extract_detail(response)
self.assertIsNone(detail)

# Test empty response
response.json.side_effect = None
response.json.return_value = {}

0 comments on commit 2474d10

Please sign in to comment.