🛡️ Sentinel: [MEDIUM] Fix Information Leakage in API Error Responses - #160
🛡️ Sentinel: [MEDIUM] Fix Information Leakage in API Error Responses#160alvin000009238 wants to merge 1 commit into
Conversation
Replaced raw exception strings with a generic error message in client responses while preserving server-side logging with exc_info=True. 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 across several routes in app/routes/grades.py and app/routes/share.py to avoid leaking internal details. Feedback suggests maintaining a consistent JSON response structure by adding a missing 'success' key in one of the routes and recommends extracting the repeated error string into a shared constant to improve maintainability.
| 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 fetch_grades_route in the same file (line 39), which includes a 'success': False key. To maintain a consistent API contract within this module, consider adding the success key to this response as well.
| return jsonify({'error': '伺服器內部錯誤'}), 500 | |
| return jsonify({'success': False, 'error': '伺服器內部錯誤'}), 500 |
| 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 generic error message '伺服器內部錯誤' is repeated across multiple routes in both grades.py and share.py. To improve maintainability and ensure consistency, consider defining this string as a constant in a shared location.
There was a problem hiding this comment.
Pull request overview
This PR mitigates potential information leakage from Flask API endpoints by replacing raw exception strings in 500 responses with a generic error message, while preserving server-side exception logging for diagnostics.
Changes:
- Replace
str(exc)in 500 JSON responses with a generic"伺服器內部錯誤"message ingradesandshareroutes. - Keep exception logging via
logger.error(..., exc_info=True)so stack traces remain available in server logs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/routes/share.py | Stops returning raw exception messages to clients for 500s on share create/update/get APIs. |
| app/routes/grades.py | Stops returning raw exception messages to clients for 500s on grade fetch and structure APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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.
Keyword-argument spacing in exc_info = True doesn’t follow PEP8 and is inconsistent with other logger calls in this file (e.g., exc_info=True at lines 67/123). Recommend changing to exc_info=True for consistency and to avoid formatter/linter noise.
| 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.
Keyword-argument spacing in exc_info = True doesn’t follow PEP8 and is inconsistent with other logger calls in this file. Recommend changing to exc_info=True for consistency.
| 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.
These handlers intentionally return a generic 500 message to avoid leaking exception details, but there’s no regression test asserting that exception text is not returned. Consider adding a route test that forces an exception (e.g., monkeypatch a dependency to raise) and asserts the response error is the generic message and does not contain the exception string.
| return jsonify({'success': True, 'message': '成績已更新', 'data': data}) | ||
| 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.
This change alters the 500 response body to a generic message for security, but there are no backend route tests covering the /api/fetch and /api/structure unhandled-exception paths. Consider adding tests that monkeypatch fetch_grades / get_structure to raise and assert the client receives the generic error (and not the exception text).
| session['structure'] = structure | ||
| return jsonify({'structure': structure}) | ||
| 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.
This 500 handler now returns a generic message (to prevent information leakage), but there isn’t a backend route test covering the /api/structure exception path. Consider adding a test that monkeypatches get_structure to raise and asserts the response contains only the generic error message (and not the exception text).
🚨 Severity: MEDIUM
💡 Vulnerability: API endpoints in
grades.pyandshare.pyreturned raw exception strings (str(exc)) to clients on unhandled 500 errors.🎯 Impact: Could leak internal system details, stack traces, or configuration information to attackers.
🔧 Fix: Replaced raw exception strings with a generic error message ("伺服器內部錯誤") in client responses while preserving server-side logging with
exc_info=True.✅ Verification: Verified by reviewing the exception handlers in the affected routes and running the test suite (
python -m pytest).PR created automatically by Jules for task 5439549021663483696 started by @alvin000009238