🛡️ Sentinel: [MEDIUM] Fix information leakage in error responses - #141
🛡️ Sentinel: [MEDIUM] Fix information leakage in error responses#141alvin000009238 wants to merge 1 commit into
Conversation
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 Chinese error message across several API routes in grades.py and share.py to prevent leaking internal system details. Feedback was provided to ensure a consistent API response schema by including a "success": false field in all error responses, aligning them with the structure used in successful responses.
| except Exception as exc: | ||
| logger.error(f'Error getting structure (API): {exc}', exc_info=True) | ||
| return jsonify({'error': str(exc)}), 500 | ||
| return jsonify({'error': '伺服器內部錯誤'}), 500 |
There was a problem hiding this comment.
The error response structure here is inconsistent with the one in fetch_grades_route (line 39). For a unified API experience, it is recommended to include the 'success': False field in all error responses, especially since the corresponding success responses (e.g., line 36) include 'success': True.
| return jsonify({'error': '伺服器內部錯誤'}), 500 | |
| return jsonify({'success': False, 'error': '伺服器內部錯誤'}), 500 |
| logger.error(f'Error creating share: {exc}', exc_info = True) | ||
| return jsonify({'error': str(exc)}), 500 | ||
| logger.error(f'Error creating share: {exc}', exc_info=True) | ||
| return jsonify({'error': '伺服器內部錯誤'}), 500 |
There was a problem hiding this comment.
The error response structure is inconsistent with the success response at line 82, which includes {'success': True}. It is recommended to include {'success': False} in the error response for consistency. Additionally, the error message is in Chinese while most other error messages in this file are in English; consider unifying the language used for API errors across the module.
| 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.
For consistency with the success response at line 147 and to maintain a unified API error schema, consider adding the 'success': False field to this error response.
| return jsonify({'error': '伺服器內部錯誤'}), 500 | |
| return jsonify({'success': False, 'error': '伺服器內部錯誤'}), 500 |
| logger.error(f'Error reading share: {exc}', exc_info = True) | ||
| return jsonify({'error': str(exc)}), 500 | ||
| logger.error(f'Error reading share: {exc}', exc_info=True) | ||
| return jsonify({'error': '伺服器內部錯誤'}), 500 |
There was a problem hiding this comment.
For consistency with the success response at line 167 and to maintain a unified API error schema, consider adding the 'success': False field to this error response.
| return jsonify({'error': '伺服器內部錯誤'}), 500 | |
| return jsonify({'success': False, 'error': '伺服器內部錯誤'}), 500 |
There was a problem hiding this comment.
Pull request overview
This PR mitigates information leakage in Flask API error handling by preventing raw exception strings from being returned to clients on HTTP 500, while keeping full exception details in server logs.
Changes:
- Replace
str(exc)in 500 JSON responses with a generic error message ('伺服器內部錯誤') in share and grades routes. - Keep detailed exception logging (
exc_info=True) and fix PEP 8 whitespace in logging calls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/routes/share.py | Sanitizes 500 error responses for share endpoints and fixes exc_info=True formatting. |
| app/routes/grades.py | Sanitizes 500 error responses for grades endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.error(f'Error creating share: {exc}', exc_info=True) | ||
| return jsonify({'error': '伺服器內部錯誤'}), 500 |
There was a problem hiding this comment.
The generic 500 error payload/message is now duplicated across multiple handlers in this module. Consider extracting a shared constant (e.g., INTERNAL_SERVER_ERROR_MESSAGE) or using a small helper to build the standard 500 JSON response so future changes (wording/i18n/format) don’t require editing multiple routes.
| except Exception as exc: | ||
| logger.error(f'Error creating share: {exc}', exc_info = True) | ||
| return jsonify({'error': str(exc)}), 500 | ||
| logger.error(f'Error creating share: {exc}', exc_info=True) | ||
| return jsonify({'error': '伺服器內部錯誤'}), 500 |
There was a problem hiding this comment.
There are existing backend route tests for /api/share, but none appear to assert that 500 responses no longer echo exception messages. Adding a regression test that forces an exception (e.g., monkeypatch write_shared_data/read_shared_data to raise) and verifies the response error is the generic string would help prevent reintroducing information leakage.
| except Exception as exc: | ||
| logger.error(f'Error fetching grades (API): {exc}', exc_info=True) | ||
| return jsonify({'success': False, 'error': str(exc)}), 500 | ||
| return jsonify({'success': False, 'error': '伺服器內部錯誤'}), 500 |
There was a problem hiding this comment.
The hard-coded internal error string is now repeated in multiple routes. Consider centralizing this message (module-level constant or shared helper) to avoid drift/typos and make it easier to adjust localization or error payload structure later.
🚨 Severity: MEDIUM
💡 Vulnerability: API error responses returned raw exception strings (
str(exc)) directly to clients on HTTP 500, potentially leaking internal stack traces or variables.🎯 Impact: Attackers could gain insights into internal workings, making it easier to craft targeted attacks.
🔧 Fix: Replaced
str(exc)with a generic string ('伺服器內部錯誤') while ensuring full error details are still securely logged on the server. Fixed whitespace violations for PEP 8 compliance.✅ Verification: Backend tests pass with
pytest.PR created automatically by Jules for task 12810421133528318492 started by @alvin000009238