🛡️ Sentinel: [MEDIUM] Fix Information Leakage - #158
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. |
There was a problem hiding this comment.
Pull request overview
This PR reduces information leakage by ensuring API consumers no longer receive raw exception strings from backend endpoints, while preserving server-side visibility via stack-trace logging.
Changes:
- Replace
str(exc)/str(e)error payloads with a generic message (伺服器內部錯誤) across multiple endpoints. - Standardize exception logging in routes to include
exc_info=True. - Adjust login failure messaging in the fetcher to avoid returning exception text.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| fetcher.py | Stop returning raw exception text to callers during login failure. |
| app/routes/system.py | Avoid leaking Redis ping exception detail in /health while adding traceback logging. |
| app/routes/share.py | Replace raw exception responses with a generic 500 error message and normalize exc_info=True usage. |
| app/routes/grades.py | Replace raw exception responses with a generic 500 error message for grades/structure APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception as e: | ||
| _log('error', username, f"Login Exception: {e}") | ||
| return False, f"登入錯誤: {str(e)}", None, None, None | ||
| return False, "登入錯誤: 伺服器內部錯誤", None, None, None |
There was a problem hiding this comment.
In this exception handler the code now returns a generic message (good for avoiding leakage), but the _log('error', ...) call does not include a traceback (it calls logger.error without exc_info). Consider logging with exc_info=True/logger.exception (or extending _log to accept exc_info) so the server logs still capture full context for debugging.
| except Exception as e: | ||
| logger.error(f'Health check: Redis ping failed: {e}') | ||
| logger.error(f'Health check: Redis ping failed: {e}', exc_info=True) | ||
| return jsonify({ | ||
| 'status': 'error', | ||
| 'redis': 'disconnected', | ||
| 'detail': str(e), | ||
| 'detail': '伺服器內部錯誤', | ||
| }), 503 |
There was a problem hiding this comment.
There’s route test coverage for /health, but not for the Redis failure path introduced/changed here. Please add a regression test that makes redis_client.ping() raise and asserts the response is 503 and that detail is the generic message (and does not echo the exception).
| 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.
Tests cover successful/validation paths for this endpoint, but there’s no regression test for this 500 handler to ensure exceptions are not reflected back to clients anymore. Consider adding a test that forces an exception in the create flow and asserts the response body contains only the generic error message.
| 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.
Consider adding a regression test for this updated 500 path (e.g., force write_shared_data/refresh_share_metadata_ttl to raise) to verify the API no longer returns the raw exception and instead returns the generic error message.
| except Exception as exc: | ||
| 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.
Consider adding a regression test for this 500 handler that forces an internal exception (e.g., make read_shared_data raise) and asserts the client receives only the generic error message (not the exception string).
| 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.
There are backend tests in the repo, but none appear to cover this updated 500 response path. Please add a regression test that makes fetch_grades(...) raise and asserts the route returns the generic error message (and does not leak the exception text).
| 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.
Please add a regression test for this exception handler (e.g., monkeypatch get_structure(...) to raise) to ensure the response remains the generic message and doesn’t regress to returning raw exception strings.
![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 updates various API routes and helper functions to replace raw exception messages with a generic '伺服器內部錯誤' (Internal Server Error) string in client responses, improving security by preventing information leakage. While the changes successfully mask sensitive details, the review highlights a potential observability issue in fetcher.py where the custom _log function fails to capture stack traces for login exceptions, hindering debugging efforts. I recommend updating the logging mechanism to ensure full stack traces are recorded internally.
| except Exception as e: | ||
| _log('error', username, f"Login Exception: {e}") | ||
| return False, f"登入錯誤: {str(e)}", None, None, None | ||
| return False, "登入錯誤: 伺服器內部錯誤", None, None, None |
There was a problem hiding this comment.
The pull request description states that full exceptions should be logged internally using exc_info=True to maintain observability while hiding sensitive details from the client. However, the logging call on line 131 uses the _log helper function, which does not support exc_info and only logs the string representation of the exception. This results in the loss of the stack trace for login failures, which contradicts the stated goal of the PR and will make debugging unexpected production issues difficult. Consider updating the _log function to support exc_info or calling logger.error(..., exc_info=True) directly in this block.
🚨 Severity: MEDIUM
💡 Vulnerability: API endpoints were returning raw exception messages directly to the client via
str(exc). This can expose internal application logic or sensitive system information.🎯 Impact: Attackers can gain insight into internal workings, dependencies, or potential weak points of the server through verbose error messages.
🔧 Fix: Replaced raw exception string returns with a generic error message ("伺服器內部錯誤") in all catch blocks within routing files, while ensuring full exceptions are still logged internally using
exc_info=True.✅ Verification: Ran
python -m pytestwhich passed successfully, indicating the functional routing paths are intact while error states are obscured from users.PR created automatically by Jules for task 2312763545488750577 started by @alvin000009238