diff --git a/custom_components/sws12500/coordinator.py b/custom_components/sws12500/coordinator.py index 1dbed5f..eaca6f9 100644 --- a/custom_components/sws12500/coordinator.py +++ b/custom_components/sws12500/coordinator.py @@ -115,7 +115,10 @@ class WeatherDataUpdateCoordinator(DataUpdateCoordinator): expected_webhook = self.config.options.get(ECOWITT_WEBHOOK_ID, "") actual_webhook = webdata.match_info.get("webhook_id", "") - if not expected_webhook or actual_webhook != expected_webhook: + # Constant-time comparison to avoid leaking the webhook id via timing. + if not expected_webhook or not hmac.compare_digest( + actual_webhook.encode("utf-8"), expected_webhook.encode("utf-8") + ): _LOGGER.error("Ecowitt: invalid webhook ID") if health: health.update_ingress_result( diff --git a/custom_components/sws12500/diagnostics.py b/custom_components/sws12500/diagnostics.py index ead10cb..5f11af7 100644 --- a/custom_components/sws12500/diagnostics.py +++ b/custom_components/sws12500/diagnostics.py @@ -8,12 +8,21 @@ from typing import Any from homeassistant.components.diagnostics import async_redact_data # pyright: ignore[reportUnknownVariableType] from homeassistant.core import HomeAssistant -from .const import API_ID, API_KEY, POCASI_CZ_API_ID, POCASI_CZ_API_KEY, WINDY_STATION_ID, WINDY_STATION_PW +from .const import ( + API_ID, + API_KEY, + ECOWITT_WEBHOOK_ID, + POCASI_CZ_API_ID, + POCASI_CZ_API_KEY, + WINDY_STATION_ID, + WINDY_STATION_PW, +) from .data import SWSConfigEntry TO_REDACT = { API_ID, API_KEY, + ECOWITT_WEBHOOK_ID, POCASI_CZ_API_ID, POCASI_CZ_API_KEY, WINDY_STATION_ID, diff --git a/custom_components/sws12500/health_coordinator.py b/custom_components/sws12500/health_coordinator.py index 5f9fd44..4af17ae 100644 --- a/custom_components/sws12500/health_coordinator.py +++ b/custom_components/sws12500/health_coordinator.py @@ -23,8 +23,10 @@ from typing import Any import aiohttp from aiohttp import ClientConnectionError import aiohttp.web +from aiohttp.web_exceptions import HTTPUnauthorized from py_typecheck import checked, checked_or +from homeassistant.components.http import KEY_AUTHENTICATED from homeassistant.components.network import async_get_source_ip from homeassistant.core import HomeAssistant from homeassistant.helpers.aiohttp_client import async_get_clientsession @@ -69,6 +71,18 @@ def _protocol_from_path(path: str) -> str: return "unknown" +def _sanitize_path(path: str) -> str: + """Strip the secret Ecowitt webhook id from a path before storing/exposing it. + + The Ecowitt endpoint is `/weatherhub/` where the id is the only + credential. Keeping the raw path in the health snapshot would leak it via the + health endpoint and diagnostics, so mask the id segment. + """ + if path.startswith(ECOWITT_URL_PREFIX + "/"): + return ECOWITT_URL_PREFIX + "/***" + return path + + def _empty_forwarding_state(enabled: bool) -> dict[str, Any]: """Build the default forwarding status payload.""" return { @@ -269,7 +283,7 @@ class HealthCoordinator(DataUpdateCoordinator): data["last_ingress"] = { "time": dt_util.utcnow().isoformat(), "protocol": _protocol_from_path(request.path), - "path": request.path, + "path": _sanitize_path(request.path), "method": request.method, "route_enabled": route_enabled, "accepted": False, @@ -294,7 +308,7 @@ class HealthCoordinator(DataUpdateCoordinator): { "time": dt_util.utcnow().isoformat(), "protocol": _protocol_from_path(request.path), - "path": request.path, + "path": _sanitize_path(request.path), "method": request.method, "accepted": accepted, "authorized": authorized, @@ -326,11 +340,23 @@ class HealthCoordinator(DataUpdateCoordinator): self._refresh_summary(data) self._commit(data) - async def health_status(self, _: aiohttp.web.Request) -> aiohttp.web.Response: + async def health_status(self, request: aiohttp.web.Request) -> aiohttp.web.Response: """Serve the current health snapshot over HTTP. + Requires Home Assistant authentication. The route is registered directly on + the aiohttp router (so it can share the dispatcher), which means HA's auth + middleware only *flags* the request - it does not block it. We therefore + enforce auth here so the snapshot (internal URLs/IPs, add-on status, last + ingress) is never exposed to unauthenticated callers. + + Auth is satisfied by a valid bearer token or a signed request, the same as + any HomeAssistantView. + The endpoint forces one refresh before returning so that the caller sees a reasonably fresh add-on status. """ + if not request.get(KEY_AUTHENTICATED, False): + raise HTTPUnauthorized + await self.async_request_refresh() return aiohttp.web.json_response(self.data, status=200) diff --git a/tests/test_health.py b/tests/test_health.py index 7840cfe..d1ef6e3 100644 --- a/tests/test_health.py +++ b/tests/test_health.py @@ -24,6 +24,7 @@ from unittest.mock import AsyncMock, MagicMock import aiohttp from aiohttp import ClientConnectionError +from aiohttp.web_exceptions import HTTPUnauthorized import pytest from pytest_homeassistant_custom_component.common import MockConfigEntry @@ -42,6 +43,7 @@ from custom_components.sws12500.const import ( from custom_components.sws12500.data import SWSRuntimeData from custom_components.sws12500.health_coordinator import HealthCoordinator from custom_components.sws12500.routes import Routes +from homeassistant.components.http import KEY_AUTHENTICATED # --------------------------------------------------------------------------- # Helpers / fixtures @@ -441,6 +443,20 @@ def test_record_dispatch_records_with_reason(hass, entry) -> None: assert coordinator.data["integration_status"] == "degraded" +def test_record_dispatch_masks_ecowitt_webhook_id(hass, entry) -> None: + coordinator = HealthCoordinator(hass, entry) + _attach_runtime_data(entry, coordinator) + + request = SimpleNamespace(path=ECOWITT_URL_PREFIX + "/supersecretid", method="POST") + coordinator.record_dispatch(request, route_enabled=True, reason=None) + + ingress = coordinator.data["last_ingress"] + assert ingress["protocol"] == "ecowitt" + # The secret webhook id must never reach the (potentially exposed) snapshot. + assert ingress["path"] == ECOWITT_URL_PREFIX + "/***" + assert "supersecretid" not in ingress["path"] + + # --------------------------------------------------------------------------- # update_ingress_result # --------------------------------------------------------------------------- @@ -520,21 +536,36 @@ def test_update_forwarding(hass, entry) -> None: # --------------------------------------------------------------------------- -async def test_health_status_endpoint(hass, entry, monkeypatch) -> None: +async def test_health_status_endpoint_authenticated(hass, entry, monkeypatch) -> None: coordinator = HealthCoordinator(hass, entry) _attach_runtime_data(entry, coordinator) # Avoid network: stub the refresh that health_status awaits. monkeypatch.setattr(coordinator, "async_request_refresh", AsyncMock(return_value=None)) - request = SimpleNamespace(path=HEALTH_URL, method="GET") - response = await coordinator.health_status(request) + # aiohttp Request is dict-like; health_status only reads KEY_AUTHENTICATED. + request = {KEY_AUTHENTICATED: True} + response = await coordinator.health_status(request) # type: ignore[arg-type] assert isinstance(response, aiohttp.web.Response) assert response.status == 200 coordinator.async_request_refresh.assert_awaited_once() +async def test_health_status_endpoint_rejects_unauthenticated(hass, entry, monkeypatch) -> None: + coordinator = HealthCoordinator(hass, entry) + _attach_runtime_data(entry, coordinator) + + refresh = AsyncMock(return_value=None) + monkeypatch.setattr(coordinator, "async_request_refresh", refresh) + + # No KEY_AUTHENTICATED flag -> unauthenticated -> 401, no refresh triggered. + with pytest.raises(HTTPUnauthorized): + await coordinator.health_status({}) # type: ignore[arg-type] + + refresh.assert_not_awaited() + + # --------------------------------------------------------------------------- # health_sensor.py module helpers # ---------------------------------------------------------------------------