Repository navigation
HA token requests never follow a redirect (P1 from the #76 review) - #78
Merged
Merged
Conversation
Codex's review of #76 (P1): urllib's default redirect handler carries every header except the body ones to the next request, Authorization included. So if the private Home Assistant, or the proxy in front of it, answered 30x to another host, CarWatch sent the long-lived HA token to a host that never passed _is_private_ha. Measured: Python 3.11, 3.12 (CI) and 3.13 forward it; 3.14 already drops it on a cross-origin redirect. - carwatch/tokenhttp.py: an opener whose redirect handler refuses (HTTPError with the 30x code and the target) instead of following. - radiation.read_ha uses it for every token request, and reports a redirect as such ("set radiation.ha.url to the final address"). - The same bug existed in mercedesme._get and _post, which send the same token: both use tokenhttp now. _post also lacked the private-host check that _get has; it has it now. - Tests: test_tokenhttp.py with two local servers (HOME redirects, OTHER logs every Authorization it sees). A negative control shows plain urllib carrying the token to OTHER; tokenhttp, radiation and both Mercedes calls must refuse, and OTHER must see nothing. Reverting either module's switch makes its tests fail. Full suite on 3.12: 235 tests OK. Not changed: room.py sends the GroupMind key to https://groupmind.one, which does not redirect; lower risk, separate decision. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This branch was successfully deployed
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
1 participant
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.
Fixes the P1 from Codex's review of #76: the Home Assistant token could follow a redirect to any host.
The bug
urllib's default redirect handler builds the next request with the original headers, except the body ones.Authorizationis carried over. If the private Home Assistant, or a reverse proxy in front of it, answers 30x to another host, CarWatch sends the long-lived HA token there. That host never passed_is_private_ha.Measured with the new negative-control test: Python 3.11, 3.12 (our CI) and 3.13 forward the token; 3.14 drops it on its own for a cross-origin redirect.
The fix
carwatch/tokenhttp.py: an opener that refuses redirects. It raisesHTTPErrorwith the 30x code and the target instead of following.radiation.read_hasends every token request through it, and reports a redirect plainly: "set radiation.ha.url to the final address".mercedesme._getand_postsend the same HA token and now usetokenhttptoo._postwas also missing the private-host check that_gethas; it has it now.Tests
tests/test_tokenhttp.py, two local servers: HOME redirects (302), OTHER records everyAuthorizationit sees.urllibcarries the token to OTHER, for GET and POST.tokenhttp,mercedesme._getandmercedesme._postmust refuse, and OTHER must see nothing._postrefuses a public HA URL before connecting.tests/test_radiation.py: a redirect reads as a redirect error, not "unreachable", and the token reaches only the configured private HA.Not in this PR
room.pysends the GroupMind key (X-API-Key) tohttps://groupmind.one, which does not redirect. It's lower risk and a separate decision.🤖 Generated with Claude Code