diff --git a/custom_components/ninlight/http.py b/custom_components/ninlight/http.py index 2813c05..7c84b93 100644 --- a/custom_components/ninlight/http.py +++ b/custom_components/ninlight/http.py @@ -3,7 +3,6 @@ from __future__ import annotations from collections.abc import Mapping -from pathlib import Path import secrets from typing import Any @@ -17,24 +16,29 @@ from .const import CONF_LAMP_BASE_URL, DOMAIN, FIRMWARE_URL_PREFIX class OtaRegistry: - """Random keys, each bound to exactly one file, valid only while its update runs.""" + """Random keys, each bound to exactly the verified bytes of one image, valid only while its + update runs. + + Bytes rather than a path: the file on disk could be replaced between verification and + download (spec ยง7 guard would otherwise apply to a file the lamp never actually receives). + """ def __init__(self) -> None: - self._paths: dict[str, Path] = {} + self._images: dict[str, bytes] = {} - def create(self, path: Path) -> str: + def create(self, data: bytes) -> str: key = secrets.token_urlsafe(32) - self._paths[key] = path + self._images[key] = data return key def revoke(self, key: str) -> None: - self._paths.pop(key, None) + self._images.pop(key, None) - def get(self, key: str) -> Path | None: - return self._paths.get(key) + def get(self, key: str) -> bytes | None: + return self._images.get(key) def __len__(self) -> int: - return len(self._paths) + return len(self._images) class NinlightFirmwareView(HomeAssistantView): @@ -49,12 +53,8 @@ class NinlightFirmwareView(HomeAssistantView): self._registry = registry async def get(self, request: web.Request, key: str) -> web.Response: - path = self._registry.get(key) - if path is None: - return web.Response(status=404) - try: - body = await self._hass.async_add_executor_job(path.read_bytes) - except OSError: + body = self._registry.get(key) + if body is None: return web.Response(status=404) return web.Response(body=body, content_type="application/octet-stream") diff --git a/custom_components/ninlight/update.py b/custom_components/ninlight/update.py index 9219efc..3ea1be9 100644 --- a/custom_components/ninlight/update.py +++ b/custom_components/ninlight/update.py @@ -28,6 +28,7 @@ from .const import ( from .coordinator import NinlightConfigEntry, NinlightCoordinator from .entity import NinlightEntity from .firmware import FirmwareImage +from .firmware import contains_ssid as _firmware_contains_ssid from .firmware import is_newer as _firmware_is_newer from .http import lamp_base_url from .shared import DATA_SHARED @@ -105,6 +106,11 @@ class NinlightUpdateEntity(NinlightEntity, UpdateEntity): @callback def _schedule_evaluation(self) -> None: + # Never evaluate while an install is running: it would poll a flashing lamp for settings + # and could churn the offer/issue underneath the install. `_async_install_locked` always + # schedules exactly one evaluation itself once the flags are cleared. + if self._attr_in_progress: + return self.coordinator.config_entry.async_create_task( self.hass, self._async_evaluate(), name="ninlight firmware evaluation" ) @@ -170,9 +176,10 @@ class NinlightUpdateEntity(NinlightEntity, UpdateEntity): async def async_install(self, version: str | None, backup: bool, **kwargs: Any) -> None: # Only the call that owns this lock may touch in_progress/ota_in_progress below: without - # this check, a second concurrent call would sail through HA's own in-progress guard (it - # only rejects installed == latest) and its `finally` would clear the flags out from under - # the first call while that one is still flashing the lamp. + # this check, a second concurrent call could reach the image verification and lamp + # re-checks before those flags are set - HA's own "already in progress" guard only helps + # once _attr_in_progress is already True - and its `finally` would clear the flags out + # from under the first call while that one is still flashing the lamp. if self._install_lock.locked(): raise _error("update_busy") async with self._install_lock: @@ -184,7 +191,11 @@ class NinlightUpdateEntity(NinlightEntity, UpdateEntity): if image is None: raise _error("no_update") base = lamp_base_url(self.hass, self.coordinator.config_entry.options) - if not await self.hass.async_add_executor_job(shared.catalog.verify, image): + # Read the bytes ONCE here and verify exactly those bytes: the view below serves this + # same in-memory copy, so nothing that happens to the file afterwards (e.g. another build + # overwriting it mid-update) can change what actually reaches the lamp. + data = await self.hass.async_add_executor_job(shared.catalog.verify_and_read, image) + if data is None: raise _error("image_invalid") # Re-check right before sending: the lamp's firmware may have changed since the offer was @@ -198,11 +209,11 @@ class NinlightUpdateEntity(NinlightEntity, UpdateEntity): raise _error("image_invalid") ssid = _ssid(info) if depends_on_compiled_wifi(settings) and not await self.hass.async_add_executor_job( - shared.catalog.contains_ssid, image, ssid + _firmware_contains_ssid, data, ssid ): raise _error("wifi_guard", ssid=ssid or "?") - key = shared.registry.create(image.path) + key = shared.registry.create(data) url = firmware_url(base, key) if len(url) > MAX_LAMP_URL_LENGTH: shared.registry.revoke(key) @@ -220,6 +231,7 @@ class NinlightUpdateEntity(NinlightEntity, UpdateEntity): self.coordinator.ota_in_progress = False await self.coordinator.async_refresh() self.async_write_ha_state() + self._schedule_evaluation() async def _async_wait_for(self, target: str, start_uptime: Any) -> None: """Wait for the lamp to report the target version, an error, or an old-version reboot. diff --git a/tests/test_http.py b/tests/test_http.py index 2604f82..2a06632 100644 --- a/tests/test_http.py +++ b/tests/test_http.py @@ -18,44 +18,38 @@ from tests_unit.images import build_image def test_registry_keys_are_unique_and_43_characters() -> None: registry = OtaRegistry() - keys = {registry.create(Path("/tmp/a.img")) for _ in range(50)} + keys = {registry.create(b"payload") for _ in range(50)} assert len(keys) == 50 assert all(len(key) == 43 for key in keys) assert len(registry) == 50 -async def test_view_serves_only_registered_keys( +async def test_view_serves_only_its_own_registered_bytes( hass: HomeAssistant, hass_client_no_auth: ClientSessionGenerator, init_integration, firmware_dir: Path, ) -> None: - path = firmware_dir / "a.img" - path.write_bytes(build_image()) registry = hass.data[DATA_SHARED].registry - other = firmware_dir / "b.img" - other.write_bytes(build_image(version="0.3.9-rc1")) - key = registry.create(path) + mine = build_image() + other = build_image(version="0.3.9-rc1") + key = registry.create(mine) + other_key = registry.create(other) client = await hass_client_no_auth() response = await client.get(f"/api/ninlight/firmware/{key}") assert response.status == 200 - assert await response.read() == path.read_bytes() + assert await response.read() == mine + + other_response = await client.get(f"/api/ninlight/firmware/{other_key}") + assert other_response.status == 200 + assert await other_response.read() == other assert (await client.get("/api/ninlight/firmware/" + "x" * 43)).status == 404 registry.revoke(key) assert (await client.get(f"/api/ninlight/firmware/{key}")).status == 404 - - -async def test_view_returns_404_when_file_vanished( - hass: HomeAssistant, - hass_client_no_auth: ClientSessionGenerator, - init_integration, - firmware_dir: Path, -) -> None: - key = hass.data[DATA_SHARED].registry.create(firmware_dir / "gone.img") - client = await hass_client_no_auth() - assert (await client.get(f"/api/ninlight/firmware/{key}")).status == 404 + # revoking one key never touches another registered under a different key + assert (await client.get(f"/api/ninlight/firmware/{other_key}")).status == 200 async def test_base_url_prefers_option(hass: HomeAssistant) -> None: diff --git a/tests/test_update.py b/tests/test_update.py index 8f071f3..bc9115a 100644 --- a/tests/test_update.py +++ b/tests/test_update.py @@ -226,6 +226,74 @@ async def test_install_serves_image_once_and_waits_for_new_version( assert init_integration.runtime_data.ota_in_progress is False +async def test_install_serves_exactly_the_verified_bytes( + hass: HomeAssistant, + hass_client_no_auth: ClientSessionGenerator, + init_integration, + mock_client: MagicMock, + lamp_info: dict[str, Any], + firmware_dir: Path, +) -> None: + """F1: the lamp must receive exactly the bytes checked (Wi-Fi guard, type, hash) at install + time, even if the file on disk is replaced by something else afterwards - e.g. a build script + that writes the no-credentials variant over the same filename mid-update. + """ + path = put(firmware_dir, "a.img", version=NEWER, content=CONTENT_WITH_SSID) + verified_bytes = path.read_bytes() + await rescan(hass) + client = await hass_client_no_auth() + served: dict[str, Any] = {} + + async def lamp_downloads(url: str) -> None: + # Swap in a different, otherwise-valid image for the same device type right before the + # lamp "fetches" it. The Wi-Fi guard already passed for `verified_bytes`; this replacement + # has no credentials at all and must never reach the lamp. + path.write_bytes(build_image(version=NEWER, content=CONTENT_WITHOUT_SSID)) + served["url"] = url + response = await client.get(urlsplit(url).path) + served["status"] = response.status + served["body"] = await response.read() + lamp_info["fw_version"] = NEWER + lamp_info["uptime"] = 4 + + mock_client.fw_update.side_effect = lamp_downloads + await install(hass) + await hass.async_block_till_done() + + assert served["status"] == 200 + assert served["body"] == verified_bytes + assert served["body"] != path.read_bytes() # the swapped-in file, not what was served + + +async def test_rescan_during_install_does_not_trigger_evaluation( + hass: HomeAssistant, + init_integration, + mock_client: MagicMock, + lamp_info: dict[str, Any], + firmware_dir: Path, +) -> None: + """M2: a catalog rescan landing while an install is in progress must not evaluate offers + against the (currently flashing) lamp - that would mean extra requests to it and could churn + the offer/issue underneath the install. + """ + put(firmware_dir, "a.img", version=NEWER, content=CONTENT_WITH_SSID) + await rescan(hass) + calls_before_rescan: dict[str, int] = {} + + async def lamp_flashes(url: str) -> None: + # a periodic catalog scan happens to land while this install is in progress + calls_before_rescan["count"] = mock_client.get_settings.await_count + await rescan(hass) + lamp_info["fw_version"] = NEWER + lamp_info["uptime"] = 4 + + mock_client.fw_update.side_effect = lamp_flashes + await install(hass) + await hass.async_block_till_done() + + assert mock_client.get_settings.await_count == calls_before_rescan["count"] + + @pytest.mark.parametrize( ("lamp_reaction", "key", "reason_part"), [ @@ -304,7 +372,12 @@ async def test_install_rechecks_wifi_right_before_sending( mock_client: MagicMock, firmware_dir: Path, lamp_settings: dict[str, Any], + monkeypatch: pytest.MonkeyPatch, ) -> None: + # Guards against a regression that would let this install slip past the re-check into the + # wait loop: without this it would hang for the full OTA_TIMEOUT_S (12 min) instead of failing + # fast with "wifi_guard". + monkeypatch.setattr(update_module, "OTA_TIMEOUT_S", 0) lamp_settings.clear() lamp_settings.update(SETTINGS_STORED) put(firmware_dir, "nocreds.img", version=NEWEST, content=CONTENT_WITHOUT_SSID) @@ -373,8 +446,9 @@ async def test_install_rejects_concurrent_calls( already True (it is a `cached_property` that HA invalidates whenever `_attr_in_progress` is set, so it is never stale) - by the time that guard would fire, a second concurrent call is already blocked by HA itself with translation_key "update_in_progress", not "update_busy". - The real gap I2 describes is *before* that: `verify`, `get_info` and `get_settings` are all - awaited before we ever set the flags, and HA's guard does not protect that window. This test + The real gap I2 describes is *before* that: image verification, `get_info` and `get_settings` + are all awaited before we ever set the flags, and HA's guard does not protect that window. This + test blocks the first `get_info()` re-check inside that window to reach it deterministically. """ put(firmware_dir, "a.img", version=NEWER, content=CONTENT_WITH_SSID)