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) <noreply@anthropic.com>ecowitt_support
parent
14466cbe60
commit
f4da5fdb27
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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])
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Reference in New Issue