Repository navigation
Add redact_uri() to hide secrets in port URLs - #127
Conversation
ESPHome URLs can carry the API key, noise PSK or password in the query string. Libraries log the port URL, so the secret ends up in logs. Add redact_uri() so that callers can log the URL safely, and use it in the missing hostname error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation safely preserves non-secret URL content, covers supported credentials, and includes focused tests and documentation.
0 open findings
What changed in this PR
Adds safe URI logging by redacting ESPHome authentication query parameters.
Changes:
- Introduces and exports
redact_uri(). - Redacts credentials in missing-hostname errors.
- Adds documentation and parameterized tests.
| File | Description |
|---|---|
serialx/common.py |
Implements URI redaction. |
serialx/__init__.py |
Exposes the helper publicly. |
serialx/platforms/serial_esphome.py |
Sanitizes hostname errors. |
tests/test_common.py |
Tests redaction behavior. |
docs/how-to/esphome.md |
Documents safe URL logging. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Reviewed at db5696c. The idea is sound, and the redaction catches everything the ESPHome backend actually reads as a secret. I checked it against parse_qs (which serial_esphome.py uses): percent-encoded names (k%65y), + in names, repeated keys and ; inside a value are all handled the same way on both sides, so nothing the backend would treat as a secret gets through, on Python 3.10 through 3.14. Tests pass locally (full suite: 1065 passed, 216 skipped, 4 xfailed), and the branch merges cleanly into current dev.
Two things I'd change, one inline and one outside the diff:
1. The urlsplit() round-trip rewrites the URI in the one place this PR uses it. See the inline note on common.py. In short: esphome:///?key=… comes out as esphome:/?key=REDACTED, so the missing-hostname error shows a URI the user never typed. Rebuilding the string around the raw query avoids that, and also stops redact_uri() from raising on a URI that urlsplit() rejects.
2. AsyncSerial.__repr__ still prints the raw URL. serialx/async_serial.py:131 is f"<AsyncSerial url={self._url!r} transport={self._transport!r}>", so repr(async_serial_for_url("esphome://host/?key=c2VjcmV0")) gives <AsyncSerial url='esphome://host/?key=c2VjcmV0' transport=None>. A library that logs its port object with %r leaks the key through serialx itself, even if it never calls redact_uri(). Since this PR is about making the URL safe to log, running self._url through redact_uri() there (when it isn't None) seems worth including. I didn't find any other __repr__, log call or exception message in serialx that carries an ESPHome URL.
Smaller points
- Nothing tests the change in
serial_esphome.py. A test that opensesphome:///?key=c2VjcmV0and checks thatInvalidSettingsErrordoesn't containc2VjcmV0would cover it. It raises before any connection attempt, so it needs no ESPHome device. - The
Run tests Python 3.12 (Linux)failure isn't caused by this PR: the job's tests finished (2217 passed, 420 skipped, 8 xfailed), every step succeeded, and the job's conclusion iscancelled. A re-run should clear it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #127 +/- ##
==========================================
+ Coverage 94.31% 94.38% +0.06%
==========================================
Files 25 25
Lines 4345 4360 +15
==========================================
+ Hits 4098 4115 +17
+ Misses 247 245 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A hand-typed ESPHome URL can carry the API key in its query string. Device libraries log the port URL, so in that case the key reaches the logs. Add a helper that replaces secret query values, so libraries can log the URL safely. The missing hostname error now uses it too.
🤖 Generated with Claude Code