Sigenergy: redact accessToken from the MQTT publish log line

[springfall2008]

This is an automated draft PR generated from issue #4920 — a maintainer should review it before merging.

Fixes #4920

Summary

SigenergyAPI._publish_mqtt() logged the full command payload dict, which both callers (set_operating_mode() and send_battery_command()) populate with the live accessToken — the same token used as the MQTT broker password. Since MQTT commands fire on every mode switch and battery command, the token landed in the app log in plaintext, repeatedly, for as long as it was valid.

This adds a SigenergyAPI.redact() static method and a SIGENERGY_LOG_REDACT_KEYS tuple, mirroring the existing DeyeAPI.redact() / SunsynkAPI.redact() pattern the report pointed at, and applies it to the one leaking log line. Redaction is log-only — the real token still goes to the broker on the wire. redact() recurses into dicts and lists because the battery command payload nests its per-system commands one level down inside a list.

Scope is deliberately limited to that log line: the other token paths were checked and are clean today — the subscription publishes (sigenergy.py:1465) log only a count, get_access_token() logs only code/msg/expiresIn, and _request() sends the token in the Authorization header rather than in the params/json_data it logs.

Testing

• tools/triage_test.sh sigenergy — all Sigenergy tests pass, including two new ones:
  • test_sigenergy_redact — masking at top level and nested inside a list, every key in the redact list, and scalar/list/None passthrough.
  • test_sigenergy_publish_mqtt_redacts_token — the broker still receives the real token while the log line contains <redacted> and not the token, non-credential payload content (systemId) is still logged, and the caller's payload dict is not mutated.
• Confirmed the new test is not vacuous: with the redaction call reverted it fails with Token must not appear in the log, and passes again once restored.
• coverage/run_pre_commit — all hooks pass (ruff, black, cspell, markdownlint) and the quick test suite is green.

[springfall2008]

Same bug class on the inbound path — the topic embeds app_key and the listener logs it raw.

The listener logs the full topic_str on every received message (sigenergy.py:1521) and in the non-JSON warning (sigenergy.py:1507), and those topics embed app_key (SIGENERGY_MQTT_TOPIC_CHANGE/PERIOD/ALARM, sigenergy.py:148-150) — a key this very list classifies as credential-bearing (it is the MQTT broker username and half of the base64 login key). So a user pasting a debug log still leaks it on every inbound line; the fix covers only the outbound publish line.

Worth either redact()-ing the topic (or the format args) in those two listener lines as part of this PR, or an explicit follow-up. The CLI tools (test_sigenergy_api, test_mqtt_connection) deliberately print only a 10-char app_key prefix, but the listener lines print the full topic.

Separately: the key/token catch-alls here will mask benign diagnostic fields in what is often the only log line for debugging a broker-side rejection (an alarm/instruction entry carrying a literal key id renders as <redacted>), while the match is case-exact, so a snake_case access_token variant would slip through. Sibling lists cover both directions (Deye adds tokenHash; Sunsynk covers snake_case plus Authorization/sign). Worth a conscious decision on both edges.

[springfall2008]

Automated reply from the triage bot (pr-cleanup).

Fixed in 0b92ad0: both listener lines now log a safe_topic with app_key masked (topic_str.replace(self.app_key, "<redacted>"), computed once after topic parsing and used for the non-JSON warning and the message line). I redacted only the topic, not the value_dict format arg, on purpose: inbound value payloads are device telemetry, and running them through redact() would amplify the over-masking problem flagged below onto every inbound state log line for no credential risk.

On the two edges of the key list — conscious decision, unchanged: the catch-alls stay, because the failure direction is asymmetric (a benign field rendering as <redacted> costs one diagnostic detail; a missed credential leaks it permanently), and case-exact matching stays to mirror the sibling integrations (deye/sunsynk are also exact-match). Verified the Sigenergy API is camelCase throughout (accessToken/refreshToken in the login/refresh responses, camelCase in the value dicts), so the snake_case variant has no live path here. Worth revisiting only if the shared-helper refactor in the third thread ever lands.

[Copilot]

🟡 Changes recommended

There are documentation/metadata mismatches in the new redaction docstring and PR scope description that should be corrected before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR addresses a security/privacy issue in the Sigenergy integration by preventing credential-like fields (notably the MQTT accessToken) from being written to logs while keeping the real values unchanged for actual API/MQTT communication.

Changes:

• Add SIGENERGY_LOG_REDACT_KEYS plus a recursive SigenergyAPI.redact() helper to mask sensitive keys in log output.
• Apply redaction to the MQTT publish log line (and other Sigenergy request/response/topic log lines touched in this diff).
• Add unit tests verifying redaction behavior (including nested structures) and that MQTT publishes still send the real token on the wire.

File summaries

File	Description
apps/predbat/sigenergy.py	Introduces redaction helper/constants and applies log-safe redaction to Sigenergy logging paths touched by the leak.
apps/predbat/tests/test_sigenergy.py	Adds tests for redaction recursion and ensures secrets don’t appear in logs while real tokens still publish.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.