🛡️ Sentinel: [MEDIUM] Fix Information Leakage in API Responses - #119
🛡️ Sentinel: [MEDIUM] Fix Information Leakage in API Responses#119alvin000009238 wants to merge 1 commit into
Conversation
Replaced raw exception strings (`str(exc)`) in API JSON error responses with a generic "伺服器內部錯誤" (Internal Server Error) message. This prevents potential exposure of internal file paths, stack traces, and database schemas to end users. Full error details are still preserved in server logs via `exc_info=True`. Affected files: - app/routes/grades.py - app/routes/share.py - app/routes/system.py Co-authored-by: alvin000009238 <107313913+alvin000009238@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces detailed exception messages with a generic internal server error message in Chinese across multiple routes to improve security and user experience. The review feedback suggests improving API consistency by ensuring that error responses in share.py include the success: False key and that the error message key in system.py is renamed from detail to error to match other endpoints.
| except Exception as exc: | ||
| logger.error(f'Error creating share: {exc}', exc_info = True) | ||
| return jsonify({'error': str(exc)}), 500 | ||
| return jsonify({'error': '伺服器內部錯誤'}), 500 |
There was a problem hiding this comment.
The error response is missing the success key, which is present in the corresponding success response (line 82). To maintain a consistent API contract for this route and facilitate easier client-side handling, it is recommended to include 'success': False in the error response.
| return jsonify({'error': '伺服器內部錯誤'}), 500 | |
| return jsonify({'success': False, 'error': '伺服器內部錯誤'}), 500 |
| except Exception as exc: | ||
| logger.error(f'Error updating share: {exc}', exc_info=True) | ||
| return jsonify({'error': str(exc)}), 500 | ||
| return jsonify({'error': '伺服器內部錯誤'}), 500 |
There was a problem hiding this comment.
The error response is missing the success key, which is present in the corresponding success response (line 147). To maintain a consistent API contract for this route, it is recommended to include 'success': False in the error response.
| return jsonify({'error': '伺服器內部錯誤'}), 500 | |
| return jsonify({'success': False, 'error': '伺服器內部錯誤'}), 500 |
| except Exception as exc: | ||
| logger.error(f'Error reading share: {exc}', exc_info = True) | ||
| return jsonify({'error': str(exc)}), 500 | ||
| return jsonify({'error': '伺服器內部錯誤'}), 500 |
There was a problem hiding this comment.
The error response is missing the success key, which is present in the corresponding success response (line 167). To maintain a consistent API contract for this route, it is recommended to include 'success': False in the error response.
| return jsonify({'error': '伺服器內部錯誤'}), 500 | |
| return jsonify({'success': False, 'error': '伺服器內部錯誤'}), 500 |
| 'status': 'error', | ||
| 'redis': 'disconnected', | ||
| 'detail': str(e), | ||
| 'detail': '伺服器內部錯誤', |
There was a problem hiding this comment.
For consistency with other API endpoints in the application (e.g., in grades.py and share.py), the error message key should be named error instead of detail. This ensures that client-side error handling logic can uniformly access the error message across all routes.
| 'detail': '伺服器內部錯誤', | |
| 'error': '伺服器內部錯誤', |
There was a problem hiding this comment.
Pull request overview
This PR mitigates CWE-209 information leakage by ensuring API endpoints no longer return raw Python exception strings in error responses, while keeping server-side logging intact for debugging.
Changes:
- Replaced
str(exc)/str(e)error details in several routes with a generic message (伺服器內部錯誤). - Updated
/healthRedis failure response to avoid exposing exception details. - Kept error logging in place to preserve debugging visibility.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/routes/system.py | Removes exception details from /health response payload while preserving status signaling. |
| app/routes/share.py | Replaces raw exception strings in share API 500 responses with a generic error message. |
| app/routes/grades.py | Replaces raw exception strings in /api/fetch and /api/structure 500 responses with a generic error message. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -82,7 +82,7 @@ def create_share_link(): | |||
| return jsonify({'success': True, 'id': share_id}) | |||
| except Exception as exc: | |||
| logger.error(f'Error creating share: {exc}', exc_info = True) | |||
There was a problem hiding this comment.
logger.error(..., exc_info = True) has spaces around =; elsewhere in this codebase the convention is exc_info=True (e.g., this file line 149 and grades/auth routes). Please standardize to avoid lint/style inconsistencies.
| logger.error(f'Error creating share: {exc}', exc_info = True) | |
| logger.error(f'Error creating share: {exc}', exc_info=True) |
| @@ -167,7 +167,7 @@ def get_shared_grades(share_id): | |||
| return jsonify({'success': True, 'data': data}) | |||
| except Exception as exc: | |||
| logger.error(f'Error reading share: {exc}', exc_info = True) | |||
There was a problem hiding this comment.
logger.error(..., exc_info = True) has spaces around =; please change to exc_info=True to match the logging style used throughout the routes and keep formatting consistent.
| logger.error(f'Error reading share: {exc}', exc_info = True) | |
| logger.error(f'Error reading share: {exc}', exc_info=True) |
| except Exception as e: | ||
| logger.error(f'Health check: Redis ping failed: {e}') | ||
| return jsonify({ | ||
| 'status': 'error', | ||
| 'redis': 'disconnected', | ||
| 'detail': str(e), | ||
| 'detail': '伺服器內部錯誤', |
There was a problem hiding this comment.
In this exception handler, logger.error(...) does not include exc_info=True, so the traceback is lost (and other routes log exceptions with exc_info=True). Since this block is being touched, consider adding exc_info=True here as well to keep diagnostic logging consistent without reintroducing response-side leakage.
🛡️ Sentinel: [MEDIUM] Fix Information Leakage in API Responses
🚨 Severity: MEDIUM
💡 Vulnerability: The application was directly returning raw Python exception strings (
str(exc)) in its 500 error API responses across multiple routes (/api/fetch,/api/structure,/api/share/*, and/health). This is a security risk (CWE-209) as exceptions can inadvertently leak sensitive internal details, such as file paths, component names, or unexpected database structures, to potentially malicious actors.🎯 Impact: An attacker could intentionally trigger errors to map out the application's internal structure, dependencies, or file system layout, aiding in further targeted attacks.
🔧 Fix: Replaced all instances of
return jsonify({'error': str(exc)}), 500with a generic, user-friendly messagereturn jsonify({'error': '伺服器內部錯誤'}), 500. The server-side logging (logger.error(..., exc_info=True)) remains untouched, ensuring developers can still debug the original issue.✅ Verification: Ran the backend Pytest suite (
pytest), which passed 100%. Verified that no frontend UI files were modified or regressions introduced.PR created automatically by Jules for task 2425258899823045488 started by @alvin000009238