Fix hassio auth IndexError on Supervisor Unix socket requests#169911
Conversation
Requests over the Supervisor Unix socket (added in #163907) are authenticated by the http auth middleware as the Supervisor user, but have an empty `peername` on the transport. The hassio auth view's `_check_access` did a TCP-style `peername[0]` IP comparison and crashed with `IndexError: string index out of range`. Skip the IP check when the request comes via the Supervisor Unix socket — the socket is the trust boundary and the existing user-ID check ensures only the Supervisor user can reach the endpoint. Also guard against a missing peername on TCP transports (reject rather than crash).
home-assistant
Bot
added
cla-signed
core
has-tests
integration: hassio
small-pr
|
Hey there @home-assistant/supervisor, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an IndexError in the Hass.io auth view when requests arrive via the Supervisor Unix socket (where peername can be empty), by skipping the IP-based peer check for Unix-socket traffic and rejecting missing peername on non-Unix-socket requests.
Changes:
- Skip the caller-IP validation for requests identified as coming over the Supervisor Unix socket.
- Add a guard so missing/empty
peernameon non-Unix-socket requests is rejected instead of causing an index error. - Add regression tests covering Unix-socket (empty
peername) and missing-peernamescenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| homeassistant/components/hassio/auth.py | Adjusts _check_access to bypass IP checks for Supervisor Unix socket requests and harden peername handling. |
| tests/components/hassio/test_auth.py | Adds parametrized tests to ensure _check_access no longer crashes on empty/missing peername. |
| if not is_supervisor_unix_socket_request(request): | ||
| hassio_ip = os.environ["SUPERVISOR"].split(":")[0] | ||
| assert request.transport | ||
| peername = request.transport.get_extra_info("peername") | ||
| if not peername or ip_address(peername[0]) != ip_address(hassio_ip): | ||
| _LOGGER.error("Invalid auth request from %s", request.remote) | ||
| raise HTTPUnauthorized |
There was a problem hiding this comment.
| if not is_supervisor_unix_socket_request(request): | |
| hassio_ip = os.environ["SUPERVISOR"].split(":")[0] | |
| assert request.transport | |
| peername = request.transport.get_extra_info("peername") | |
| if not peername or ip_address(peername[0]) != ip_address(hassio_ip): | |
| _LOGGER.error("Invalid auth request from %s", request.remote) | |
| raise HTTPUnauthorized | |
| if not is_supervisor_unix_socket_request(request): | |
| hassio_ip = os.environ["SUPERVISOR"].split(":")[0] | |
| if ( | |
| not (transport := request.transport) | |
| or not (peername := transport.get_extra_info("peername")) | |
| or ip_address(peername[0]) != ip_address(hassio_ip) | |
| ): | |
| _LOGGER.error("Invalid auth request from %s", request.remote) | |
| raise HTTPUnauthorized |
I'd overprefer a walrus operator here to patch out the assert. What do you think? :)
There was a problem hiding this comment.
Note that the assert is existing code.
There was a problem hiding this comment.
True, merely spotted a potential improvement. :)
There was a problem hiding this comment.
We use the same pattern (assert) also in other places in the hassio integration.
From what I understand that happens if the connection closed. I am not sure how realistic this is in this particular case, the close must have happened between the request coming in and processing here 🤔
I'd rather prefer to address all sites at once, if we decide to change how to handle request.transport being None.
There was a problem hiding this comment.
That makes sense. I agree this does not need to block this PR, especially if the same pattern exists elsewhere.
If it's there purely to satisfy mypy, it should be fine. If we want to continuously check this in runtime, it would justify its own dedicated PR. :)
There was a problem hiding this comment.
It got added with #74603 as a mypy defense, so I assume the latter.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
|
||
| request = MagicMock() | ||
| request.transport.get_extra_info.return_value = peername | ||
| request.__getitem__.side_effect = lambda key: user if key is KEY_HASS_USER else None |
There was a problem hiding this comment.
Consider moving the side effect to a test parameter that is a lambda.
Address review feedback on the regression test:
- Use `@pytest.mark.usefixtures("hassio_stubs")` instead of taking it
as an unused parameter (silences Pylance "not accessed").
- Split the parametrized test with an `if/else` branch into two
separate tests — one per behavior — per CLAUDE.md guidance to
avoid branching in tests.
- Extract the shared setup into a small helper.
Per @MartinHjelmare's review feedback: - Merge the two split tests back into a single parametrized test using `contextlib.ExitStack as DefaultContext` for the success case and `pytest.raises(HTTPUnauthorized)` for the failure case, removing the conditional branch in the test body. - Move the `__getitem__` side effect into a parameter (lambda).
Per Copilot review feedback: short string identity comparisons rely on CPython interning and aren't guaranteed across implementations.
| ( | ||
| None, | ||
| False, | ||
| lambda user, key: user if key == KEY_HASS_USER else None, |
There was a problem hiding this comment.
I thought the condition in the lambda matched the parameter cases. Maybe I misunderstood that?
There was a problem hiding this comment.
If the lambda is always the same we don't need to have it as a test parameter. But I was thinking we could remove the condition in the lambda and just return the correct thing depending on the test case.
There was a problem hiding this comment.
Yeah we always check with Supervisor user. I guess we could argue testing with another user for completeness. But the lambda stays the same anyways, it's about which key returns the username.
But yeah lambda doesn't make sense in the parameter list, I'll remove it again.
There was a problem hiding this comment.
I guess we could argue testing with another user for completeness.
Hm, makes the test more complex, and it's (test) feature creep. This is about Supervisor user only, but covering Unix domain socket. So I left it at that.
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
I'll approve preemptively here. The above question can be solved as needed or not.
The lambda was identical across both parameter rows, so it doesn't need to be a parameter. `_check_access` only ever subscripts the request with `KEY_HASS_USER`, so a single `__getitem__.return_value = user` is sufficient and removes the conditional from the mock side effect.
|
Test failure unrelated. |
Breaking change
Proposed change
Requests over the Supervisor Unix socket (added in #163907) are authenticated by the http auth middleware as the Supervisor user, but have an empty
peernameon the transport. The hassio auth view's_check_accessdid a TCP-stylepeername[0]IP comparison and crashed withIndexError: string index out of range.Skip the IP check when the request comes via the Supervisor Unix socket — the socket is the trust boundary and the existing user-ID check ensures only the Supervisor user can reach the endpoint. Also guard against a missing peername on TCP transports (reject rather than crash).
This is related to #163907.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: