From f4da5fdb27f534f83879dc79e6bc3ce60300045d Mon Sep 17 00:00:00 2001 From: SchiZzA Date: Sun, 21 Jun 2026 18:48:50 +0200 Subject: [PATCH] fix(security): harden forwarders, cap native ecowitt entities, reject empty creds - S8: received_data now rejects empty configured credentials (IncorrectDataError) and present-but-empty incoming credentials (HTTPUnauthorized) instead of relying solely on the config flow. Credential validation extracted into _validate_credentials() (also resolves the C901 complexity warning and unifies the WU/WSLink branches). - S6: cap auto-created native Ecowitt entities (MAX_NATIVE_ECOWITT_SENSORS) to bound entity-registry growth from a fabricated multi-key payload. - S5: store only the exception class name in Windy/Pocasi last_error (surfaced via entity attributes; str(ex) could embed the request URL). - S7: verified safe - the native-sensor INFO log is parametrized (%s), so no format-string injection; left as-is. 308 passing, 100% coverage; ruff + basedpyright clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- custom_components/sws12500/coordinator.py | 129 +++++++++++----------- custom_components/sws12500/ecowitt.py | 14 ++- custom_components/sws12500/pocasti_cz.py | 4 +- custom_components/sws12500/windy_func.py | 4 +- tests/test_ecowitt_bridge.py | 15 +++ tests/test_received_data.py | 22 ++++ 6 files changed, 119 insertions(+), 69 deletions(-) diff --git a/custom_components/sws12500/coordinator.py b/custom_components/sws12500/coordinator.py index 87461a3..f71b218 100644 --- a/custom_components/sws12500/coordinator.py +++ b/custom_components/sws12500/coordinator.py @@ -92,6 +92,68 @@ class WeatherDataUpdateCoordinator(DataUpdateCoordinator): except AttributeError: return None + def _validate_credentials( + self, + data: dict[str, Any], + webdata: aiohttp.web.Request, + *, + wslink: bool, + health: HealthCoordinator | None, + ) -> None: + """Validate station credentials for the legacy / WSLink endpoint. + + Raises HTTPUnauthorized (missing/empty/wrong credentials) or IncorrectDataError + (integration not configured); returns None on success. + """ + id_key, pw_key = ("wsid", "wspw") if wslink else ("ID", "PASSWORD") + + if id_key not in data or pw_key not in data: + _LOGGER.error("Invalid request. No security data provided!") + if health: + health.update_ingress_result(webdata, accepted=False, authorized=False, reason="missing_credentials") + raise HTTPUnauthorized + + id_data = data.get(id_key, "") + key_data = data.get(pw_key, "") + + if (_id := checked(self.config.options.get(API_ID), str)) is None: + _LOGGER.error("We don't have API ID set! Update your config!") + if health: + health.update_ingress_result(webdata, accepted=False, authorized=None, reason="config_missing_api_id") + raise IncorrectDataError + + if (_key := checked(self.config.options.get(API_KEY), str)) is None: + _LOGGER.error("We don't have API KEY set! Update your config!") + if health: + health.update_ingress_result(webdata, accepted=False, authorized=None, reason="config_missing_api_key") + raise IncorrectDataError + + # Defense-in-depth: reject empty configured/incoming credentials so this handler + # is self-protecting regardless of how options were set. + if not _id or not _key: + _LOGGER.error("API ID/KEY is empty! Update your config!") + if health: + health.update_ingress_result( + webdata, accepted=False, authorized=None, reason="config_missing_credentials" + ) + raise IncorrectDataError + + if not id_data or not key_data: + _LOGGER.error("Unauthorised access! Empty credentials.") + if health: + health.update_ingress_result(webdata, accepted=False, authorized=False, reason="unauthorized") + raise HTTPUnauthorized + + # Constant-time comparison; both operands are always compared (no short-circuit) + # and encoded to bytes so non-ASCII credentials are handled safely. + id_ok = hmac.compare_digest(id_data.encode("utf-8"), _id.encode("utf-8")) + key_ok = hmac.compare_digest(key_data.encode("utf-8"), _key.encode("utf-8")) + if not (id_ok & key_ok): + _LOGGER.error("Unauthorised access!") + if health: + health.update_ingress_result(webdata, accepted=False, authorized=False, reason="unauthorized") + raise HTTPUnauthorized + async def received_ecowitt_data(self, webdata: aiohttp.web.Request) -> aiohttp.web.Response: """Handle incoming Ecowitt webhook payload. @@ -203,72 +265,7 @@ class WeatherDataUpdateCoordinator(DataUpdateCoordinator): health = self._health_coordinator() - if not _wslink and ("ID" not in data or "PASSWORD" not in data): - _LOGGER.error("Invalid request. No security data provided!") - if health: - health.update_ingress_result( - webdata, - accepted=False, - authorized=False, - reason="missing_credentials", - ) - raise HTTPUnauthorized - - if _wslink and ("wsid" not in data or "wspw" not in data): - _LOGGER.error("Invalid request. No security data provided!") - if health: - health.update_ingress_result( - webdata, - accepted=False, - authorized=False, - reason="missing_credentials", - ) - raise HTTPUnauthorized - - if _wslink: - id_data = data.get("wsid", "") - key_data = data.get("wspw", "") - else: - id_data = data.get("ID", "") - key_data = data.get("PASSWORD", "") - - if (_id := checked(self.config.options.get(API_ID), str)) is None: - _LOGGER.error("We don't have API ID set! Update your config!") - if health: - health.update_ingress_result( - webdata, - accepted=False, - authorized=None, - reason="config_missing_api_id", - ) - raise IncorrectDataError - - if (_key := checked(self.config.options.get(API_KEY), str)) is None: - _LOGGER.error("We don't have API KEY set! Update your config!") - if health: - health.update_ingress_result( - webdata, - accepted=False, - authorized=None, - reason="config_missing_api_key", - ) - raise IncorrectDataError - - # Constant-time comparison to avoid leaking credential length/content via timing. - # Both operands are compared even if the first fails, so the branch order doesn't - # short-circuit. Encode to bytes so non-ASCII credentials are handled safely. - id_ok = hmac.compare_digest(id_data.encode("utf-8"), _id.encode("utf-8")) - key_ok = hmac.compare_digest(key_data.encode("utf-8"), _key.encode("utf-8")) - if not (id_ok & key_ok): - _LOGGER.error("Unauthorised access!") - if health: - health.update_ingress_result( - webdata, - accepted=False, - authorized=False, - reason="unauthorized", - ) - raise HTTPUnauthorized + self._validate_credentials(data, webdata, wslink=_wslink, health=health) remaped_items: dict[str, str] = remap_wslink_items(data) if _wslink else remap_items(data) diff --git a/custom_components/sws12500/ecowitt.py b/custom_components/sws12500/ecowitt.py index 4788414..2646192 100644 --- a/custom_components/sws12500/ecowitt.py +++ b/custom_components/sws12500/ecowitt.py @@ -13,7 +13,7 @@ from __future__ import annotations from datetime import datetime import logging -from typing import Any +from typing import Any, Final from aioecowitt import EcoWittListener, EcoWittSensor, EcoWittSensorTypes @@ -33,6 +33,10 @@ _LOGGER = logging.getLogger(__name__) # we need to know which key is internally covered. _MAPPED_ECOWITT_KEYS: set[str] = set(REMAP_ECOWITT_COMPAT.keys()) +# Upper bound on auto-created native Ecowitt entities. Bounds entity-registry growth +# from an (authenticated) sender that fabricates many distinct sensor keys. +MAX_NATIVE_ECOWITT_SENSORS: Final = 64 + # aioecowitt sensor type to HA device class + unit # We cover most common types, additional will be covered later. STYPE_TO_HA: dict[EcoWittSensorTypes, tuple[SensorDeviceClass | None, str | None, SensorStateClass | None]] = { @@ -222,6 +226,14 @@ class EcowittBridge: _LOGGER.debug("Ecowitt sensor %s discovered but platform not ready yet", sensor.key) return + if len(self._know_native_keys) >= MAX_NATIVE_ECOWITT_SENSORS: + _LOGGER.warning( + "Reached the cap of %s native Ecowitt sensors; ignoring new key %s", + MAX_NATIVE_ECOWITT_SENSORS, + sensor.key, + ) + return + self._know_native_keys.add(sensor.key) entity = EcoWittNativeSensor(sensor) self._add_entities_cb([entity]) diff --git a/custom_components/sws12500/pocasti_cz.py b/custom_components/sws12500/pocasti_cz.py index 618f746..872146c 100644 --- a/custom_components/sws12500/pocasti_cz.py +++ b/custom_components/sws12500/pocasti_cz.py @@ -157,7 +157,9 @@ class PocasiPush: except ClientError as ex: self.last_status = "client_error" - self.last_error = str(ex) + # Store only the exception class - last_error is surfaced via entity + # attributes; str(ex) could embed the request URL. + self.last_error = type(ex).__name__ _LOGGER.critical("Invalid response from Pocasi Meteo: %s", str(ex)) self.invalid_response_count += 1 if self.invalid_response_count >= 3: diff --git a/custom_components/sws12500/windy_func.py b/custom_components/sws12500/windy_func.py index 3851d0c..c0b6c94 100644 --- a/custom_components/sws12500/windy_func.py +++ b/custom_components/sws12500/windy_func.py @@ -295,7 +295,9 @@ class WindyPush: except ClientError as ex: self.last_status = "client_error" - self.last_error = str(ex) + # Store only the exception class - last_error is surfaced via entity + # attributes; str(ex) could embed the request URL. + self.last_error = type(ex).__name__ _LOGGER.critical( "Invalid response from Windy: %s. Will try again later, max retries before disabling resend function: %s", str(ex), diff --git a/tests/test_ecowitt_bridge.py b/tests/test_ecowitt_bridge.py index 796f9c4..71d7ec9 100644 --- a/tests/test_ecowitt_bridge.py +++ b/tests/test_ecowitt_bridge.py @@ -370,3 +370,18 @@ async def test_set_add_entities_flushes_pending_unmapped_sensors() -> None: # The previously-skipped unmapped sensors are created now. assert created assert all(isinstance(e, EcoWittNativeSensor) for e in created) + + +def test_on_new_sensor_respects_cap() -> None: + """Native entity creation is capped to bound entity-registry growth.""" + from custom_components.sws12500.ecowitt import MAX_NATIVE_ECOWITT_SENSORS + + bridge = _make_bridge() + cb = MagicMock() + bridge.set_add_entities(cb) + # Pretend we already created the maximum number of native sensors. + bridge._know_native_keys = {f"k{i}" for i in range(MAX_NATIVE_ECOWITT_SENSORS)} + + bridge._on_new_sensor(_make_sensor(key="pm25_ch1")) + + cb.assert_not_called() diff --git a/tests/test_received_data.py b/tests/test_received_data.py index 720fd19..16eaab4 100644 --- a/tests/test_received_data.py +++ b/tests/test_received_data.py @@ -509,3 +509,25 @@ async def test_register_path_switching_logic_is_exercised_via_routes(monkeypatch """Sanity: constants exist and are distinct (helps guard tests relying on them).""" assert DEFAULT_URL != WSLINK_URL assert DOMAIN == "sws12500" + + +@pytest.mark.asyncio +async def test_received_data_empty_configured_credentials_raises_incorrect_data(hass, monkeypatch): + """Empty configured API ID/KEY is treated as missing config (defense-in-depth).""" + entry = _make_entry(wslink=False, api_id="", api_key="key") + entry.runtime_data.health_coordinator = SimpleNamespace(update_ingress_result=MagicMock()) + coordinator = WeatherDataUpdateCoordinator(hass, entry) + with pytest.raises(IncorrectDataError): + await coordinator.received_data(_RequestStub(query={"ID": "id", "PASSWORD": "key"})) # type: ignore[arg-type] + entry.runtime_data.health_coordinator.update_ingress_result.assert_called_once() + + +@pytest.mark.asyncio +async def test_received_data_empty_incoming_credentials_raises_unauthorized(hass, monkeypatch): + """Present-but-empty incoming credentials are rejected before the digest compare.""" + entry = _make_entry(wslink=False, api_id="id", api_key="key") + entry.runtime_data.health_coordinator = SimpleNamespace(update_ingress_result=MagicMock()) + coordinator = WeatherDataUpdateCoordinator(hass, entry) + with pytest.raises(HTTPUnauthorized): + await coordinator.received_data(_RequestStub(query={"ID": "", "PASSWORD": ""})) # type: ignore[arg-type] + entry.runtime_data.health_coordinator.update_ingress_result.assert_called_once()