fix(security): return generic error and log internally in RoleRestAPI.get_list#41295
fix(security): return generic error and log internally in RoleRestAPI.get_list#41295rusackas wants to merge 2 commits into
Conversation
….get_list RoleRestAPI.get_list returned str(e) directly in the 500 response body and made no logger call, so raw ORM/driver error text could surface to the caller while the error was invisible in structured logs. Log the exception server-side via logger.exception and return a generic message to the caller instead of the raw exception text. Adds an integration test asserting an unexpected error yields a generic 500 body (no raw exception detail) and is logged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code Review Agent Run #4c43efActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The suggestion to add type hints to the new test method is correct, as it improves code quality and consistency. You can resolve this by updating the method signature to include def test_show_roles_unexpected_error_returns_generic_message(self: Any) -> None:There are no other comments on this pull request. Would you like me to implement this change for you? tests/integration_tests/security/api_tests.py |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41295 +/- ##
==========================================
- Coverage 64.34% 64.34% -0.01%
==========================================
Files 2653 2653
Lines 144952 144953 +1
Branches 33433 33433
==========================================
- Hits 93273 93271 -2
- Misses 49995 49996 +1
- Partials 1684 1686 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…n get_list The broad db.session.query patch raised inside the @Protect() auth check (get_public_role) before get_list ran, so the exception bypassed the handler's try/except and leaked. Patch selectinload, used only inside get_list's query construction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #3a5495Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
RoleRestAPI.get_list(/api/v1/security/roles/search/) caught unexpected errors with a hand-writtenexcept Exceptionthat returnedstr(e)directly in the HTTP 500 response body and made no logging call. This meant raw ORM/driver error text could surface to the caller, while the same error was invisible in the structured logging pipeline.This change:
logger.exception(...)so operators get the full detail and stack trace."An unexpected error occurred"message to the caller instead of the raw exception text.The endpoint remains gated behind
@protect()/list_roles; this only changes what an unexpected-error response body contains and ensures the error is recorded.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend error-handling behavior.
TESTING INSTRUCTIONS
Integration test added in
tests/integration_tests/security/api_tests.py(test_show_roles_unexpected_error_returns_generic_message):get_list.logger.exceptionwas called.Run:
pytest tests/integration_tests/security/api_tests.py -k unexpected_errorADDITIONAL INFORMATION