Skip to content

🛡️ Sentinel: [MEDIUM] Fix Information Leakage #158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alvin000009238 wants to merge 1 commit into from
Closed

🛡️ Sentinel: [MEDIUM] Fix Information Leakage #158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/routes/grades.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def fetch_grades_route():
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
Comment on lines 37 to +39

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.


@bp.route('/api/structure', methods=['GET'])
Expand Down Expand Up @@ -64,7 +64,7 @@ def get_structure_route():
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
Comment on lines 65 to +67

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.



10 changes: 5 additions & 5 deletions app/routes/share.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ def create_share_link():
write_share_metadata(redis_client, share_id, student_no, share_ttl)
return jsonify({'success': True, 'id': share_id})
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
Comment on lines 83 to +85

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.


@bp.route('/api/share/<share_id>', methods=['PUT'])
Expand Down Expand Up @@ -147,7 +147,7 @@ def update_share_link(share_id):
return jsonify({'success': True, 'id': share_id})
except Exception as exc:
logger.error(f'Error updating share: {exc}', exc_info=True)
return jsonify({'error': str(exc)}), 500
return jsonify({'error': '伺服器內部錯誤'}), 500
Comment on lines 148 to +150

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.


@bp.route('/api/share/<share_id>', methods=['GET'])
Expand All @@ -166,8 +166,8 @@ 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)
return jsonify({'error': str(exc)}), 500
logger.error(f'Error reading share: {exc}', exc_info=True)
return jsonify({'error': '伺服器內部錯誤'}), 500
Comment on lines 168 to +170

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.


@bp.route('/share/<share_id>')
Expand Down
4 changes: 2 additions & 2 deletions app/routes/system.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,11 @@ def health_check():
redis_client.ping()
redis_status = 'connected'
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
Comment on lines 52 to 58

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.

return jsonify({'status': 'ok', 'redis': redis_status}), 200
Expand Down
2 changes: 1 addition & 1 deletion fetcher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ def login_and_get_tokens(self, username, password):

except Exception as e:
_log('error', username, f"Login Exception: {e}")
return False, f"登入錯誤: {str(e)}", None, None, None
return False, "登入錯誤: 伺服器內部錯誤", None, None, None
Comment on lines 130 to +132

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

@gemini-code-assist gemini-code-assist Bot Apr 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.


def get_structure_via_api(self, cookies, student_no, token, session=None):
"""Fetch structure using requests"""
Expand Down
Loading