Fix WebSocket handshake failure for chargers missing Connection: Upgrade header#1933
Open
vavallee wants to merge 5 commits into
Open
Fix WebSocket handshake failure for chargers missing Connection: Upgrade header#1933vavallee wants to merge 5 commits into
vavallee wants to merge 5 commits into
Conversation
…ade header Some charger firmware (confirmed: Autel MaxiCharger US AC LW10-N14, firmware v1.38.00 / v1.22.00) sends a WebSocket opening handshake without the required `Connection: Upgrade` header. websockets 14+ strictly enforces this header in `ServerProtocol.accept()` and raises `InvalidUpgrade: missing Connection header`, causing the handshake to fail and the charger to be permanently unable to connect. Fix: add a `process_request` hook to the `websockets.serve()` call that injects `Connection: upgrade` when the header is absent, before the library's validation runs. A warning is logged so operators are aware and can chase a firmware update from the charger vendor.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a WebSocket Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Hook as _fix_missing_connection_header
participant WS as websockets.serve / handshake
participant App as CentralSystem
Client->>WS: HTTP WebSocket Upgrade request (may lack Connection)
WS->>Hook: call process_request(request)
alt Connection header missing
Hook->>Hook: log WARNING "Connection: Upgrade"
Hook->>WS: mutate request.headers (set Connection: upgrade)
else Connection header present
Hook->>WS: return None (no change)
end
WS->>App: continue handshake / establish WebSocket
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom_components/ocpp/api.py (1)
206-217:⚠️ Potential issue | 🟡 MinorAdd unit test for the Connection header injection callback.
_fix_missing_connection_headeris never invoked during tests becausewebsockets.asyncio.server.serveis fully mocked intests/conftest.py. Since this callback works around a real charger firmware bug, add a unit test that constructs aRequestobject with missingConnectionheader and verifies the injection behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custom_components/ocpp/api.py` around lines 206 - 217, Add a unit test that imports the _fix_missing_connection_header function and constructs a websockets Request object lacking the "Connection" header, call _fix_missing_connection_header with that Request and assert the function injects "Connection: Upgrade" (or equivalent expected value) into the headers and returns the expected None/continue result; ensure the test does not mock websockets.serve so the callback itself is exercised and reference _fix_missing_connection_header and the Request class from the websockets package to locate the code under test.
🧹 Nitpick comments (2)
custom_components/ocpp/api.py (2)
196-203: Consider adding defensive error handling for header mutation.The code assumes
request.headersis mutable. While this works withwebsockets.datastructures.Headers, if the implementation changes or mutation fails, the error would be opaque. Consider wrapping in a try/except:🛡️ Optional: Add defensive error handling
if not request.headers.get_all("Connection"): _LOGGER.warning( "Charger at %s sent a WebSocket upgrade request without the " "required 'Connection: Upgrade' header. Injecting header as " "a workaround — consider updating the charger firmware.", connection.remote_address, ) - request.headers["Connection"] = "upgrade" + try: + request.headers["Connection"] = "upgrade" + except TypeError: + _LOGGER.error( + "Failed to inject Connection header — headers may be immutable" + ) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custom_components/ocpp/api.py` around lines 196 - 203, Wrap the header mutation that sets request.headers["Connection"] = "upgrade" in a try/except to handle and log any exceptions if request.headers is not mutable; inside the except, call _LOGGER.exception or _LOGGER.error including connection.remote_address and a short message that mutation failed so the upgrade workaround is skipped safely. Ensure this change is applied where the current warning is emitted (the block referencing request.headers, _LOGGER.warning, and connection.remote_address) so the code keeps the warning but also defensively handles mutation failures.
187-195: Docstring formatting: summary line should be single line per PEP 257.The static analysis tool flags D205. The summary spans multiple lines before the blank line separator.
📝 Optional: Fix docstring formatting
async def _fix_missing_connection_header( connection: ServerConnection, request: Request ) -> Response | None: - """Work around a bug in some charger firmware (e.g. Autel MaxiCharger - US AC LW10-N14, firmware v1.38.00/v1.22.00) that omits the required - Connection: Upgrade header from the WebSocket opening handshake. + """Inject missing Connection header for non-compliant charger firmware. + Works around a bug in some charger firmware (e.g. Autel MaxiCharger + US AC LW10-N14, firmware v1.38.00/v1.22.00) that omits the required + Connection: Upgrade header from the WebSocket opening handshake. websockets 14+ strictly validates this header and raises InvalidUpgrade when it is absent, preventing the charger from ever connecting. We inject the header before the library validates it so that the handshake can proceed normally. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custom_components/ocpp/api.py` around lines 187 - 195, The docstring starting with "Work around a bug in some charger firmware..." should follow PEP 257 by making the first line a single-line summary, then a blank line, and then the longer explanatory paragraph; update the docstring in custom_components.ocpp.api (the workaround docstring) to a one-line summary (e.g., "Work around chargers that omit the required Connection: Upgrade header.") followed by a blank line and the detailed explanation that follows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@custom_components/ocpp/api.py`:
- Around line 206-217: Add a unit test that imports the
_fix_missing_connection_header function and constructs a websockets Request
object lacking the "Connection" header, call _fix_missing_connection_header with
that Request and assert the function injects "Connection: Upgrade" (or
equivalent expected value) into the headers and returns the expected
None/continue result; ensure the test does not mock websockets.serve so the
callback itself is exercised and reference _fix_missing_connection_header and
the Request class from the websockets package to locate the code under test.
---
Nitpick comments:
In `@custom_components/ocpp/api.py`:
- Around line 196-203: Wrap the header mutation that sets
request.headers["Connection"] = "upgrade" in a try/except to handle and log any
exceptions if request.headers is not mutable; inside the except, call
_LOGGER.exception or _LOGGER.error including connection.remote_address and a
short message that mutation failed so the upgrade workaround is skipped safely.
Ensure this change is applied where the current warning is emitted (the block
referencing request.headers, _LOGGER.warning, and connection.remote_address) so
the code keeps the warning but also defensively handles mutation failures.
- Around line 187-195: The docstring starting with "Work around a bug in some
charger firmware..." should follow PEP 257 by making the first line a
single-line summary, then a blank line, and then the longer explanatory
paragraph; update the docstring in custom_components.ocpp.api (the workaround
docstring) to a one-line summary (e.g., "Work around chargers that omit the
required Connection: Upgrade header.") followed by a blank line and the detailed
explanation that follows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 029c30af-cfed-4720-80cb-bce24468e894
📒 Files selected for processing (1)
custom_components/ocpp/api.py
drc38
approved these changes
Mar 29, 2026
vavallee
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1933 +/- ##
==========================================
+ Coverage 94.94% 94.95% +0.01%
==========================================
Files 12 12
Lines 2946 2952 +6
==========================================
+ Hits 2797 2803 +6
Misses 149 149 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… unit tests Makes the function directly importable for testing without needing to instantiate CentralSystem or mock websockets.serve. Adds two async unit tests: one verifying header injection + warning log when Connection is absent, one verifying the header is left intact when already present. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
custom_components/ocpp/api.py (1)
46-54: Docstring formatting: add blank line after summary.Static analysis (D205) flags that a blank line is required between the summary line and the description. The summary should ideally be a single line.
📝 Proposed docstring fix
async def _fix_missing_connection_header( connection: ServerConnection, request: Request ) -> Response | None: - """Work around a bug in some charger firmware (e.g. Autel MaxiCharger - US AC LW10-N14, firmware v1.38.00/v1.22.00) that omits the required - Connection: Upgrade header from the WebSocket opening handshake. - - websockets 14+ strictly validates this header and raises + """Inject missing Connection header for non-compliant charger firmware. + + Works around a bug in some charger firmware (e.g. Autel MaxiCharger + US AC LW10-N14, firmware v1.38.00/v1.22.00) that omits the required + Connection: Upgrade header from the WebSocket opening handshake. + websockets 14+ strictly validates this header and raises InvalidUpgrade when it is absent, preventing the charger from ever connecting. We inject the header before the library validates it so that the handshake can proceed normally. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custom_components/ocpp/api.py` around lines 46 - 54, The top-level (module) docstring in api.py needs a D205-compliant format: make the first line a single short summary, add a blank line, then place the longer description (the workaround details about injecting the Connection: Upgrade header for certain chargers) on subsequent lines; update the triple-quoted module docstring accordingly so there is an empty line between the summary and the rest of the text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_api_paths.py`:
- Around line 435-448: The test
test_fix_missing_connection_header_leaves_existing_intact claims "no warning is
logged" but doesn't verify logs; update the test to accept the pytest caplog
fixture, call _fix_missing_connection_header using caplog (e.g., caplog at
WARNING level), and assert that no WARNING-level records were emitted after the
call; reference the test function name and the target function
_fix_missing_connection_header so you add the caplog parameter and an assertion
like assert not any(r.levelname == "WARNING" for r in caplog.records) (or
equivalent) to ensure no warning was logged.
---
Nitpick comments:
In `@custom_components/ocpp/api.py`:
- Around line 46-54: The top-level (module) docstring in api.py needs a
D205-compliant format: make the first line a single short summary, add a blank
line, then place the longer description (the workaround details about injecting
the Connection: Upgrade header for certain chargers) on subsequent lines; update
the triple-quoted module docstring accordingly so there is an empty line between
the summary and the rest of the text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b6b427c-95bd-491e-b8b8-d955a5ba1470
📒 Files selected for processing (2)
custom_components/ocpp/api.pytests/test_api_paths.py
test_fix_missing_connection_header_leaves_existing_intact claimed "no warning is logged" in its docstring but never verified it. Added caplog fixture, wrapped the call in caplog.at_level(WARNING), and asserted no WARNING-level records were emitted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
vavallee
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
Add missing blank lines after function definition (E302) and restructure docstring to have a one-line summary (D205). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Some charger firmware omits the required
Connection: UpgradeHTTP header from the WebSocket opening handshake. This has been confirmed with:websockets14+ (used by this integration) strictly validates this header inServerProtocol.accept()and raises:The handshake fails immediately, the server sends
CLOSE 1000, and the charger retries indefinitely — it can never connect.Fix
Add a
process_requesthook to thewebsockets.serve()call. This hook runs before the library validates the handshake headers, giving us the opportunity to injectConnection: upgradewhen it is absent.The hook logs a
WARNINGso operators know the charger has a firmware bug and can pursue an update from their charger vendor.Notes
Connectionheader is completely absent (not just malformed), so well-behaved chargers are unaffectedwebsockets.datastructures.Headersis mutable, so the injection works cleanlySummary by CodeRabbit