From 7c9343f90de7f8c52dfd79eae4c323663104475b Mon Sep 17 00:00:00 2001 From: SchiZzA Date: Sun, 26 Jul 2026 00:39:15 +0200 Subject: [PATCH] fix(sws12500): harden credential validation, URL fallback and push timeouts - Reject blank or whitespace-only PWS credentials in both the config flow and the options flow. INVALID_CREDENTIALS only held placeholder strings, so an empty API ID or key passed validation, the entry was created, and _validate_credentials then rejected every incoming packet. - Fall back to "UNKNOWN" host/port when get_url raises NoURLAvailableError. The values are shown as Ecowitt setup instructions only, so an unresolvable Home Assistant URL must not make the step unreachable. - Catch TimeoutError alongside ClientError in the Windy and Pocasi push paths. A timeout is not a ClientError and used to escape into the webhook handler, answering the station with HTTP 500 even though the measured data was already stored. - Add regression tests for all three and pin that 0-5 battery descriptions really reach the WSLink sensor platform. --- custom_components/sws12500/config_flow.py | 47 ++++++-- custom_components/sws12500/pocasti_cz.py | 5 +- custom_components/sws12500/windy_func.py | 5 +- tests/test_battery_classification.py | 24 +++- tests/test_config_flow.py | 140 ++++++++++++++++++++++ tests/test_pocasi_push.py | 34 ++++++ tests/test_windy_push.py | 37 ++++++ 7 files changed, 277 insertions(+), 15 deletions(-) diff --git a/custom_components/sws12500/config_flow.py b/custom_components/sws12500/config_flow.py index 3485db6..412bb46 100644 --- a/custom_components/sws12500/config_flow.py +++ b/custom_components/sws12500/config_flow.py @@ -9,9 +9,9 @@ import voluptuous as vol from yarl import URL from homeassistant.config_entries import ConfigEntry, ConfigFlow, ConfigFlowResult, OptionsFlow -from homeassistant.core import callback +from homeassistant.core import HomeAssistant, callback from homeassistant.helpers import selector -from homeassistant.helpers.network import get_url +from homeassistant.helpers.network import NoURLAvailableError, get_url from .conflicts import ERROR_MUTUALLY_EXCLUSIVE from .const import ( @@ -42,6 +42,31 @@ from .const import ( _PASSWORD_SELECTOR = selector.TextSelector(selector.TextSelectorConfig(type=selector.TextSelectorType.PASSWORD)) +def _is_invalid_credential(value: Any) -> bool: + """Return True when a PWS/WSLink credential is unusable. + + Blank (or whitespace-only) values are as useless as the placeholder strings in + `INVALID_CREDENTIALS`: `_validate_credentials` rejects every incoming packet if + either option is empty, so the entry would be created but never receive data. + """ + return not isinstance(value, str) or not value.strip() or value in INVALID_CREDENTIALS + + +def _ha_url_placeholders(hass: HomeAssistant) -> tuple[str, str]: + """Return (host, port) of the Home Assistant URL for the Ecowitt instructions. + + `get_url` raises `NoURLAvailableError` when HA cannot resolve any of its own + URLs. These values are shown as setup instructions only, so fall back to a + placeholder instead of letting the config flow abort. + """ + try: + url: URL = URL(get_url(hass)) + except NoURLAvailableError: + return "UNKNOWN", "UNKNOWN" + + return url.host or "UNKNOWN", str(url.port) + + class ConfigOptionsFlowHandler(OptionsFlow): """Handle WeatherStation ConfigFlow.""" @@ -160,9 +185,9 @@ class ConfigOptionsFlowHandler(OptionsFlow): # legacy one while Ecowitt is active would corrupt those entities. if self.ecowitt.get(ECOWITT_ENABLED): errors["base"] = ERROR_MUTUALLY_EXCLUSIVE - elif user_input[API_ID] in INVALID_CREDENTIALS or user_input.get(API_ID, "") == "": + elif _is_invalid_credential(user_input.get(API_ID)): errors[API_ID] = "valid_credentials_api" - elif user_input[API_KEY] in INVALID_CREDENTIALS or user_input.get(API_KEY, "") == "": + elif _is_invalid_credential(user_input.get(API_KEY)): errors[API_KEY] = "valid_credentials_key" elif user_input[API_KEY] == user_input[API_ID]: errors["base"] = "valid_credentials_match" @@ -258,8 +283,7 @@ class ConfigOptionsFlowHandler(OptionsFlow): else: return self.async_create_entry(title=DOMAIN, data=self.retain_data(user_input)) - url: URL = URL(get_url(self.hass)) - host = url.host or "UNKNOWN" + host, port = _ha_url_placeholders(self.hass) ecowitt_schema = { vol.Required( @@ -277,7 +301,7 @@ class ConfigOptionsFlowHandler(OptionsFlow): data_schema=vol.Schema(ecowitt_schema), description_placeholders={ "url": host, - "port": str(url.port), + "port": port, "webhook_id": webhook, }, errors=errors, @@ -358,9 +382,9 @@ class ConfigFlowHandler(ConfigFlow, domain=DOMAIN): if user_input is None: return self.async_show_form(step_id="pws", data_schema=vol.Schema(self.pws_schema), errors=errors) - if user_input[API_ID] in INVALID_CREDENTIALS: + if _is_invalid_credential(user_input.get(API_ID)): errors[API_ID] = "valid_credentials_api" - elif user_input[API_KEY] in INVALID_CREDENTIALS: + elif _is_invalid_credential(user_input.get(API_KEY)): errors[API_KEY] = "valid_credentials_key" elif user_input[API_KEY] == user_input[API_ID]: errors["base"] = "valid_credentials_match" @@ -383,8 +407,7 @@ class ConfigFlowHandler(ConfigFlow, domain=DOMAIN): if user_input is None: webhook = secrets.token_hex(8) - url: URL = URL(get_url(self.hass)) - host = url.host or "UNKNOWN" + host, port = _ha_url_placeholders(self.hass) ecowitt_schema = { vol.Required(ECOWITT_WEBHOOK_ID, default=webhook): str, @@ -396,7 +419,7 @@ class ConfigFlowHandler(ConfigFlow, domain=DOMAIN): data_schema=vol.Schema(ecowitt_schema), description_placeholders={ "url": host, - "port": str(url.port), + "port": port, "webhook_id": webhook, }, ) diff --git a/custom_components/sws12500/pocasti_cz.py b/custom_components/sws12500/pocasti_cz.py index ef9c54b..7cf2275 100644 --- a/custom_components/sws12500/pocasti_cz.py +++ b/custom_components/sws12500/pocasti_cz.py @@ -180,7 +180,10 @@ class PocasiPush: _LOGGER.critical(POCASI_CZ_UNEXPECTED) await self._disable_pocasi(POCASI_CZ_UNEXPECTED) - except ClientError as ex: + # TimeoutError is not a ClientError: an `async_timeout`/`asyncio` timeout would + # otherwise escape into the webhook handler and answer the station with HTTP 500, + # even though the measured data was already stored. + except (ClientError, TimeoutError) as ex: self.last_status = "client_error" # Store only the exception class - last_error is surfaced via entity # attributes; str(ex) could embed the request URL. diff --git a/custom_components/sws12500/windy_func.py b/custom_components/sws12500/windy_func.py index b983389..54412db 100644 --- a/custom_components/sws12500/windy_func.py +++ b/custom_components/sws12500/windy_func.py @@ -311,7 +311,10 @@ class WindyPush: reason=f"Unable to send data to Windy ({WINDY_MAX_RETRIES} times). Disabling resend option for now. Please check your Windy configuration and enable this feature afterwards." ) - except ClientError as ex: + # TimeoutError is not a ClientError: an `async_timeout`/`asyncio` timeout would + # otherwise escape into the webhook handler and answer the station with HTTP 500, + # even though the measured data was already stored. + except (ClientError, TimeoutError) as ex: self.last_status = "client_error" # Store only the exception class - last_error is surfaced via entity # attributes; str(ex) could embed the request URL. diff --git a/tests/test_battery_classification.py b/tests/test_battery_classification.py index 90532d1..99821ea 100644 --- a/tests/test_battery_classification.py +++ b/tests/test_battery_classification.py @@ -81,13 +81,35 @@ def test_level_descriptions_generated_from_battery_non_binary() -> None: def test_level_batteries_are_not_also_plain_sensors() -> None: - """A 0-5 battery must appear exactly once in each platform's description set.""" + """A 0-5 battery must not be hand-defined on top of its generated description. + + `BATTERY_LEVEL_SENSORS` is spliced into `SENSOR_TYPES_WSLINK`, so one occurrence + there is the generated one and is expected; `SENSOR_TYPES_WEATHER_API` does not + splice them in, so zero is expected there. "At most once" is therefore the + invariant that holds for both - a second entry is a duplicate entity. + + Complete *absence* of a description is caught by + `test_level_descriptions_generated_from_battery_non_binary`, which pins the + generated set to `BATTERY_NON_BINARY` exactly. + """ for sensor_types in (SENSOR_TYPES_WSLINK, SENSOR_TYPES_WEATHER_API): keys = [desc.key for desc in sensor_types] for key in BATTERY_NON_BINARY: assert keys.count(key) <= 1, f"{key} defined more than once" +def test_level_batteries_reach_the_sensor_platform() -> None: + """Every 0-5 battery must actually be spliced into the WSLink platform set. + + The generated tuple existing is not enough - if the `*BATTERY_LEVEL_SENSORS` + splice were dropped, the classification tests would still pass while the + percentage entities silently disappeared. + """ + keys = [desc.key for desc in SENSOR_TYPES_WSLINK] + for key in BATTERY_NON_BINARY: + assert key in keys, f"{key} is classified as a 0-5 battery but has no sensor description" + + def test_binary_batteries_are_not_plain_sensors_too() -> None: """0/1 batteries belong to the binary platform only. diff --git a/tests/test_config_flow.py b/tests/test_config_flow.py index db0a058..cda9194 100644 --- a/tests/test_config_flow.py +++ b/tests/test_config_flow.py @@ -29,6 +29,7 @@ from custom_components.sws12500.const import ( WSLINK_ADDON_PORT, ) from homeassistant import config_entries +from homeassistant.helpers.network import NoURLAvailableError @pytest.mark.asyncio @@ -127,6 +128,81 @@ async def test_config_flow_user_invalid_credentials_api_key( assert result2["errors"][API_KEY] == "valid_credentials_key" +@pytest.mark.parametrize("blank", ["", " "]) +@pytest.mark.parametrize("field", [API_ID, API_KEY]) +@pytest.mark.asyncio +async def test_config_flow_user_rejects_blank_credentials( + hass, enable_custom_integrations, field, blank +) -> None: + """Blank PWS credentials must not create an entry. + + `INVALID_CREDENTIALS` holds placeholder strings only, so an empty (or + whitespace-only) value used to pass validation. The entry was created but + `_validate_credentials` then rejected every incoming packet, leaving the + integration permanently without data. + """ + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + form = await hass.config_entries.flow.async_configure( + result["flow_id"], user_input={"next_step_id": "pws"} + ) + assert form["step_id"] == "pws" + + user_input = { + API_ID: "ok_id", + API_KEY: "ok_key", + WSLINK: False, + DEV_DBG: False, + } + user_input[field] = blank + + result2 = await hass.config_entries.flow.async_configure( + form["flow_id"], user_input=user_input + ) + assert result2["type"] == "form" + assert result2["step_id"] == "pws" + expected = "valid_credentials_api" if field is API_ID else "valid_credentials_key" + assert result2["errors"][field] == expected + + +@pytest.mark.parametrize("blank", ["", " "]) +@pytest.mark.parametrize("field", [API_ID, API_KEY]) +@pytest.mark.asyncio +async def test_options_flow_basic_rejects_blank_credentials( + hass, enable_custom_integrations, field, blank +) -> None: + """Same blank-credential rule applies when the legacy endpoint is re-enabled.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={}, + options={LEGACY_ENABLED: False, ECOWITT_ENABLED: False}, + ) + entry.add_to_hass(hass) + + init = await hass.config_entries.options.async_init(entry.entry_id) + form = await hass.config_entries.options.async_configure( + init["flow_id"], user_input={"next_step_id": "basic"} + ) + assert form["step_id"] == "basic" + + user_input = { + API_ID: "ok_id", + API_KEY: "ok_key", + WSLINK: False, + DEV_DBG: False, + LEGACY_ENABLED: True, + } + user_input[field] = blank + + result = await hass.config_entries.options.async_configure( + init["flow_id"], user_input=user_input + ) + assert result["type"] == "form" + expected = "valid_credentials_api" if field is API_ID else "valid_credentials_key" + assert result["errors"][field] == expected + + @pytest.mark.asyncio async def test_config_flow_user_invalid_credentials_match( hass, enable_custom_integrations @@ -410,6 +486,70 @@ async def test_options_flow_ecowitt_uses_get_url_placeholders_and_webhook_defaul assert done["data"][ECOWITT_ENABLED] is True +@pytest.mark.asyncio +async def test_options_flow_ecowitt_survives_no_url_available( + hass, enable_custom_integrations +) -> None: + """`get_url` raising must not abort the Ecowitt options step. + + The host/port are shown as setup instructions only. HA can fail to resolve any + of its own URLs (no internal/external URL configured), and letting that escape + made the Ecowitt setup unreachable. + """ + entry = MockConfigEntry( + domain=DOMAIN, + data={}, + options={ECOWITT_WEBHOOK_ID: "", ECOWITT_ENABLED: False, LEGACY_ENABLED: False}, + ) + entry.add_to_hass(hass) + + init = await hass.config_entries.options.async_init(entry.entry_id) + with patch( + "custom_components.sws12500.config_flow.get_url", + side_effect=NoURLAvailableError, + ): + form = await hass.config_entries.options.async_configure( + init["flow_id"], user_input={"next_step_id": "ecowitt"} + ) + assert form["type"] == "form" + placeholders = form.get("description_placeholders") or {} + assert placeholders["url"] == "UNKNOWN" + assert placeholders["port"] == "UNKNOWN" + assert placeholders["webhook_id"] + + +@pytest.mark.asyncio +async def test_config_flow_ecowitt_survives_no_url_available( + hass, enable_custom_integrations +) -> None: + """The initial Ecowitt step stays usable when HA cannot resolve its own URL.""" + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + with patch( + "custom_components.sws12500.config_flow.get_url", + side_effect=NoURLAvailableError, + ): + form = await hass.config_entries.flow.async_configure( + result["flow_id"], user_input={"next_step_id": "ecowitt"} + ) + assert form["type"] == "form" + placeholders = form.get("description_placeholders") or {} + assert placeholders["url"] == "UNKNOWN" + assert placeholders["port"] == "UNKNOWN" + + # The step is still completable without a resolvable URL. + done = await hass.config_entries.flow.async_configure( + form["flow_id"], + user_input={ + ECOWITT_WEBHOOK_ID: placeholders["webhook_id"], + ECOWITT_ENABLED: True, + }, + ) + assert done["type"] == "create_entry" + assert done["data"][ECOWITT_ENABLED] is True + + @pytest.mark.asyncio async def test_options_flow_wslink_port_setup(hass, enable_custom_integrations) -> None: """The WSLink add-on port step shows a form and stores the port.""" diff --git a/tests/test_pocasi_push.py b/tests/test_pocasi_push.py index 0dbfab9..d220797 100644 --- a/tests/test_pocasi_push.py +++ b/tests/test_pocasi_push.py @@ -338,6 +338,40 @@ async def test_push_data_to_server_client_error_increments_and_disables_after_th ) +@pytest.mark.asyncio +async def test_push_data_to_server_timeout_is_handled_like_client_error( + monkeypatch, hass +): + """A network timeout must not escape into the webhook handler. + + `TimeoutError` is not a subclass of `ClientError`, so it used to propagate out + of `push_data_to_server`, through the awaiting coordinator, and answer the + station with HTTP 500 - even though the measured data was already stored. + """ + entry = _make_entry() + pp = PocasiPush(hass, entry) + + monkeypatch.setattr( + "custom_components.sws12500.pocasti_cz.update_options", + _write_through_update_options(entry), + ) + monkeypatch.setattr("custom_components.sws12500.pocasti_cz._LOGGER.critical", MagicMock()) + + session = _FakeSession(exc=TimeoutError("timed out")) + monkeypatch.setattr( + "custom_components.sws12500.pocasti_cz.async_get_clientsession", + lambda _h: session, + ) + + pp.next_update = dt_util.utcnow() - timedelta(seconds=1) + await pp.push_data_to_server({"x": 1}, "WU") + + assert pp.last_status == "client_error" + assert pp.last_error == "TimeoutError" + assert pp.invalid_response_count == 1 + assert pp.enabled is True + + def test_verify_response_logs_debug_when_logger_enabled(monkeypatch, hass): entry = _make_entry(logger=True) pp = PocasiPush(hass, entry) diff --git a/tests/test_windy_push.py b/tests/test_windy_push.py index 6a6d7db..fe116d2 100644 --- a/tests/test_windy_push.py +++ b/tests/test_windy_push.py @@ -449,6 +449,43 @@ async def test_push_data_to_windy_client_error_increments_and_disables_after_thr ) +@pytest.mark.asyncio +async def test_push_data_to_windy_timeout_is_handled_like_client_error( + monkeypatch, hass +): + """A network timeout must not escape into the webhook handler. + + `TimeoutError` is not a subclass of `ClientError`, so it used to propagate out + of `push_data_to_windy`, through the awaiting coordinator, and answer the + station with HTTP 500 - even though the measured data was already stored. + """ + entry = _make_entry() + wp = WindyPush(hass, entry) + wp.next_update = dt_util.utcnow() - timedelta(seconds=1) + + monkeypatch.setattr( + "custom_components.sws12500.windy_func.update_options", AsyncMock(return_value=True) + ) + monkeypatch.setattr("custom_components.sws12500.windy_func._LOGGER.critical", MagicMock()) + monkeypatch.setattr( + "custom_components.sws12500.windy_func.persistent_notification.async_create", + MagicMock(), + ) + + session = _FakeSession(exc=TimeoutError("timed out")) + monkeypatch.setattr( + "custom_components.sws12500.windy_func.async_get_clientsession", + lambda _h: session, + ) + + ok = await wp.push_data_to_windy({"a": "b"}) + + assert ok is True + assert wp.last_status == "client_error" + assert wp.last_error == "TimeoutError" + assert wp.invalid_response_count == 1 + + @pytest.mark.asyncio async def test_push_data_to_windy_client_error_disable_failure_logs_debug( monkeypatch, hass