🛡️ Sentinel: [MEDIUM] Fix Information Leakage in API Routes - #100
🛡️ Sentinel: [MEDIUM] Fix Information Leakage in API Routes#100alvin000009238 wants to merge 1 commit into
Conversation
Replaced generic exception messages returned to the client with a safe, generic '伺服器內部錯誤' string to prevent internal details from leaking via `str(exc)`. Full exception traces are still logged server-side. 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. |
There was a problem hiding this comment.
Pull request overview
This PR mitigates information leakage in several API routes by ensuring unexpected exceptions no longer return raw exception strings to clients, while preserving full error context in server logs.
Changes:
- Replaced
str(exc)in JSON error responses with a generic'伺服器內部錯誤'message for/api/share,/api/share/<share_id>,/api/fetch, and/api/structure. - Kept detailed exception logging via
logger.error(..., exc_info=True)so debugging information remains available server-side.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/routes/share.py | Sanitizes error responses for share-link creation/lookup endpoints to prevent exception detail leakage. |
| app/routes/grades.py | Sanitizes error responses for grade fetch and structure endpoints to prevent exception detail leakage. |
💡 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.
logger.error(..., exc_info = True) uses spaces around = in the keyword argument, which is inconsistent with the rest of the codebase (and earlier in this file, e.g. exc_info=True above). Please change to exc_info=True for PEP 8/style consistency.
| 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.
logger.error(..., exc_info = True) uses spaces around = in the keyword argument; please change to exc_info=True to match the established logging style in this repo (and other calls in this file).
![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 raw exception messages with a generic localized error message across several API endpoints in grades.py and share.py to avoid leaking internal details. Feedback suggests including a success: False key in the error responses within share.py to maintain consistency with the success responses and the patterns used in other routes.
| 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 success response for this endpoint (line 62) includes a success: True key. To maintain consistency and ensure the frontend can handle responses uniformly, the error response should include 'success': False. This also aligns with the pattern used in app/routes/grades.py (line 39).
| 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 success response for this endpoint (line 78) includes a success: True key. To maintain consistency and ensure the frontend can handle responses uniformly, the error response should include 'success': False. This also aligns with the pattern used in app/routes/grades.py (line 39).
| return jsonify({'error': '伺服器內部錯誤'}), 500 | |
| return jsonify({'success': False, 'error': '伺服器內部錯誤'}), 500 |
🚨 Severity: MEDIUM
💡 Vulnerability: Information Leakage. The
/api/share,/api/share/<share_id>,/api/fetch, and/api/structureendpoints caught generic exceptions and returnedstr(exc)in the JSON response under theerrorkey. This could leak internal stack traces, DB queries, or configuration details to a malicious actor.🎯 Impact: Attackers could gain deeper insight into the server's internal state and architecture, aiding in the discovery of further vulnerabilities.
🔧 Fix: Replaced the
str(exc)return payload with the generic string '伺服器內部錯誤' (Internal Server Error) inapp/routes/grades.pyandapp/routes/share.py. Full exception details are preserved in the server logs usinglogger.error(..., exc_info=True).✅ Verification: Created an ad-hoc test script to simulate a mock exception, verifying that the endpoint successfully returned the safe '伺服器內部錯誤' string instead of the sensitive mock error text. Existing backend and frontend tests continue to pass.
PR created automatically by Jules for task 7549753262348765942 started by @alvin000009238