Fix: Bugs and Improvements from Code Review#73
Conversation
Addressed the following issues: 1. Fixed lost subscriptions during active reconnect in mqtt/client.py 2. Fixed resource leak in ensure_device_info_cached by adding unsubscribe_device_feature 3. Fixed misleading units in README.rst example 4. Exposed session property in NavienAuthClient and used it in NavienAPIClient 5. Removed static unit from recirc_dhw_flow_rate in models.py
Addressed 4 high severity alerts regarding clear-text logging of sensitive information (topics). - Fixed direct logging of topic in subscriptions.py - Refactored redact_topic to use separate variable for better static analysis - Added redaction to debug logs
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…sensitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Corrected direct topic logging in subscriptions.py that was missed or incorrectly edited in previous attempt. Now consistently using redact_topic(topic) and fixed line length.
Ran 'tox -e lint' locally and fixed formatting issues in subscriptions.py caught by ruff format check.
…sensitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Replaced specific topic logging with generic messages in unsubscription logic to prevent potential sensitive data leakage and satisfy CodeQL security checks.
Applied ruff format to fix linting errors introduced by previous CodeQL fix.
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple issues identified in a code review, focusing on fixing critical bugs, resource leaks, and improving code quality across the MQTT client, API client, and documentation.
Changes:
- Fixed critical MQTT reconnection bug by adding
resubscribe_all()call after active reconnection - Fixed resource leak by implementing proper unsubscription mechanism for device feature callbacks
- Improved logging consistency by redacting sensitive MQTT topics
- Enhanced API client to use the new public
sessionproperty from auth client - Fixed data model to remove static unit specification for dynamic flow rate field
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/nwp500/mqtt/client.py | Added resubscribe_all() call during active reconnection and implemented unsubscribe_device_feature() method |
| src/nwp500/mqtt/subscriptions.py | Added topic redaction in logging, implemented unsubscribe_device_feature() with handler tracking via _original_callback |
| src/nwp500/mqtt/utils.py | Introduced intermediate variable for topic redaction to improve clarity |
| src/nwp500/auth.py | Exposed session property for public access |
| src/nwp500/api_client.py | Updated to use new session property with fallback for compatibility |
| src/nwp500/models.py | Removed hardcoded unit from recirc_dhw_flow_rate to support dynamic units |
| README.rst | Removed hardcoded temperature unit from example to reflect dynamic unit support |
| self._message_handlers.pop(topic, None) | ||
|
|
||
| _logger.info(f"Unsubscribed from '{topic}'") | ||
| _logger.info("Unsubscribed from topic (redacted)") |
There was a problem hiding this comment.
For consistency with line 235, this should use the redacted topic format: _logger.info(f"Unsubscribed from topic: {redact_topic(topic)}") instead of a generic message. This provides better debugging information while still protecting sensitive data.
| _logger.info("Unsubscribed from topic (redacted)") | |
| _logger.info(f"Unsubscribed from topic: {redact_topic(topic)}") |
| _logger.error( | ||
| f"Failed to unsubscribe from '{redact_topic(topic)}': {e}" | ||
| ) | ||
| _logger.error(f"Failed to unsubscribe from topic (redacted): {e}") |
There was a problem hiding this comment.
For consistency with line 235, this should use the redacted topic format: _logger.error(f"Failed to unsubscribe from topic: {redact_topic(topic)}: {e}") instead of a generic message. The redact_topic function is already imported and used elsewhere in this file.
| _logger.error(f"Failed to unsubscribe from topic (redacted): {e}") | |
| _logger.error( | |
| f"Failed to unsubscribe from topic: {redact_topic(topic)}: {e}" | |
| ) |
| "Unsubscribe from topic (redacted) was " | ||
| "cancelled but will complete in background" |
There was a problem hiding this comment.
For consistency with line 235, this should use the redacted topic format: "Unsubscribe from topic: {redact_topic(topic)} was cancelled but will complete in background" instead of a generic message.
| "Unsubscribe from topic (redacted) was " | |
| "cancelled but will complete in background" | |
| "Unsubscribe from topic: %s was cancelled but will complete in background", | |
| redact_topic(topic), |
src/nwp500/api_client.py
Outdated
| # Use public session property if available, fall back to private | ||
| # _session for backward compatibility or if using older auth client | ||
| auth_session = getattr(auth_client, "session", None) | ||
| if auth_session is None: | ||
| auth_session = getattr(auth_client, "_session", None) | ||
|
|
||
| self._session = session or auth_session |
There was a problem hiding this comment.
The comment mentions "backward compatibility or if using older auth client", but since the session property is being added in this same PR (auth.py lines 636-639), there are no "older auth clients" that lack this property yet. This fallback logic is unnecessary. Consider simplifying to just self._session = session or auth_client.session since the property will always be available.
| # Use public session property if available, fall back to private | |
| # _session for backward compatibility or if using older auth client | |
| auth_session = getattr(auth_client, "session", None) | |
| if auth_session is None: | |
| auth_session = getattr(auth_client, "_session", None) | |
| self._session = session or auth_session | |
| self._session = session or auth_client.session |
src/nwp500/mqtt/utils.py
Outdated
| # Use a new variable to avoid confusion in static analysis | ||
| redacted_topic = topic |
There was a problem hiding this comment.
The introduction of the redacted_topic variable is unnecessary and doesn't solve any actual static analysis issues. The function should directly reassign to topic as it did before. This adds complexity without benefit and makes the code less idiomatic. The comment mentions "confusion in static analysis" but there's no evidence of any such issue, and reassigning parameters is a common Python pattern.
| # Redact topic for logging to avoid leaking sensitive information | ||
| # (device IDs). We perform this check early to ensure we don't log raw | ||
| # topics. | ||
| # Note: CodeQL may still flag this if we log the variable derived from | ||
| # topic, so we use a generic message. | ||
| _logger.info("Unsubscribing from topic (redacted)") |
There was a problem hiding this comment.
This logging approach is inconsistent with the subscribe method at line 171, which logs "Subscribing to topic: {redact_topic(topic)}" showing the redacted topic. For consistency and better diagnostics, consider using _logger.info(f"Unsubscribing from topic: {redact_topic(topic)}") instead of a completely generic message. This maintains useful debugging information while still protecting sensitive data.
Incorporated feedback from PR #73: - api_client.py: Simplified session property fallback logic. - mqtt/utils.py: Reverted unnecessary variable introduction in redact_topic. - mqtt/subscriptions.py: Restored consistent logging format with topic redaction while maintaining security posture.
Resolved CodeQL 'Clear-text logging of sensitive information' alerts by using generic log messages for sensitive topics, as taint analysis flagged redaction as insufficient. Also fixed formatting issues to pass lint checks.
This PR addresses several issues identified in the code review (reference/Code Review_ Bugs and Improvements.md).
resubscribe_all()call during active reconnection in MQTT client.ensure_device_info_cachedusing newunsubscribe_device_featuremethod.°Funit in README example.sessionproperty inNavienAuthClientand updatedNavienAPIClientto use it.recirc_dhw_flow_rateto support dynamic units.Validation:
make ci-lintpassed.pytestpassed (402 tests).