diff --git a/server/src/ledgrab/api/routes/devices.py b/server/src/ledgrab/api/routes/devices.py index 936266d..f704479 100644 --- a/server/src/ledgrab/api/routes/devices.py +++ b/server/src/ledgrab/api/routes/devices.py @@ -42,6 +42,30 @@ logger = get_logger(__name__) router = APIRouter() +def _sanitize_url_for_log(url: str) -> str: + """Strip userinfo + fragment from a URL so secrets don't reach logs. + + The pair endpoint receives a user-supplied URL on every call; if a + future driver ever accepts ``scheme://user:pass@host`` form the + credentials would land in logs without this guard. + """ + if not url: + return "" + try: + from urllib.parse import urlparse, urlunparse + + parsed = urlparse(url) + # urlparse stores userinfo in `netloc`; rebuild without it. + if parsed.hostname: + netloc = parsed.hostname + if parsed.port: + netloc = f"{netloc}:{parsed.port}" + return urlunparse((parsed.scheme, netloc, parsed.path, parsed.params, parsed.query, "")) + except ValueError: + pass + return url + + def _device_to_response(device) -> DeviceResponse: """Convert a Device to DeviceResponse.""" return DeviceResponse( @@ -66,15 +90,14 @@ def _device_to_response(device) -> DeviceResponse: ddp_color_order=device.ddp_color_order, espnow_peer_mac=device.espnow_peer_mac, espnow_channel=device.espnow_channel, - hue_username=device.hue_username, - hue_client_key=device.hue_client_key, + hue_paired=bool(device.hue_username and device.hue_client_key), hue_entertainment_group_id=device.hue_entertainment_group_id, yeelight_min_interval_ms=device.yeelight_min_interval_ms, wiz_min_interval_ms=device.wiz_min_interval_ms, lifx_min_interval_ms=device.lifx_min_interval_ms, govee_min_interval_ms=device.govee_min_interval_ms, opc_channel=device.opc_channel, - nanoleaf_token=device.nanoleaf_token, + nanoleaf_paired=bool(device.nanoleaf_token), nanoleaf_min_interval_ms=device.nanoleaf_min_interval_ms, spi_speed_hz=device.spi_speed_hz, spi_led_type=device.spi_led_type, @@ -333,16 +356,23 @@ async def pair_device( except ValueError as exc: raise HTTPException(status_code=422, detail=str(exc)) except Exception as exc: + # Strip userinfo before logging so a `scheme://user:pass@host` URL + # never lands in the logs (no shipped driver uses userinfo today, + # but the pattern is a foot-gun for the next driver author -- + # caught by review MEDIUM #9). Also keep exc_info=False so a + # provider stack trace that may include response bytes from a + # hostile receiver doesn't end up in the file either. + safe_url = _sanitize_url_for_log(body.url) logger.warning( - "Pairing failed for %s at %s: %s", + "Pairing failed for %s at %s: %s: %s", body.device_type, - body.url, + safe_url, + type(exc).__name__, exc, - exc_info=True, ) raise HTTPException( status_code=502, - detail=f"Pairing failed for {body.device_type} at {body.url}.", + detail=f"Pairing failed for {body.device_type} at {safe_url}.", ) if not isinstance(fields, dict): @@ -520,16 +550,21 @@ async def update_device( existing = store.get_device(device_id) is_group = existing.device_type == "group" - # Normalize a WLED URL the same way we do on create: infer http/https - # when the user typed a bare host. Done via a local rather than - # mutating the request DTO so the input is preserved for any future - # caller that inspects it. + # Normalize URL the same way we do on create: + # * always rstrip trailing slashes (so PUT-with-trailing-/ matches + # POST-with-trailing-/ in the stored value -- caught by review HIGH #6) + # * only WLED gets http/https scheme inference; other schemes + # (yeelight://, lifx://, opc://, ddp://, …) pass through. + # Done via a local rather than mutating the request DTO so the + # input is preserved for any future caller that inspects it. normalized_url = update_data.url - if existing.device_type == "wled" and update_data.url: - raw_url = update_data.url.rstrip("/") - normalized_url = infer_http_scheme(raw_url) - if normalized_url != raw_url: - logger.debug("Inferred WLED URL scheme: %r -> %r", raw_url, normalized_url) + if update_data.url: + normalized_url = update_data.url.rstrip("/") + if existing.device_type == "wled": + inferred = infer_http_scheme(normalized_url) + if inferred != normalized_url: + logger.debug("Inferred WLED URL scheme: %r -> %r", normalized_url, inferred) + normalized_url = inferred # Group-only field overrides (led_count auto-recompute) are accumulated # here too so the update_data Pydantic model is not mutated in place. diff --git a/server/src/ledgrab/api/schemas/devices.py b/server/src/ledgrab/api/schemas/devices.py index 3e8bd9f..b0e9e9a 100644 --- a/server/src/ledgrab/api/schemas/devices.py +++ b/server/src/ledgrab/api/schemas/devices.py @@ -409,8 +409,14 @@ class DeviceResponse(BaseModel): ddp_color_order: int = Field(default=1, description="DDP color order code (1 = RGB)") espnow_peer_mac: str = Field(default="", description="ESP-NOW peer MAC address") espnow_channel: int = Field(default=1, description="ESP-NOW WiFi channel") - hue_username: str = Field(default="", description="Hue bridge username") - hue_client_key: str = Field(default="", description="Hue entertainment client key") + hue_paired: bool = Field( + default=False, + description=( + "Whether the Hue bridge has been paired (i.e. a username/client_key " + "is on file). The actual credentials are intentionally not exposed " + "in the response -- to re-pair, delete and re-add the device." + ), + ) hue_entertainment_group_id: str = Field(default="", description="Hue entertainment group ID") yeelight_min_interval_ms: int = Field( default=500, description="Yeelight client-side rate limit in ms" @@ -419,7 +425,14 @@ class DeviceResponse(BaseModel): lifx_min_interval_ms: int = Field(default=50, description="LIFX client-side rate limit in ms") govee_min_interval_ms: int = Field(default=50, description="Govee client-side rate limit in ms") opc_channel: int = Field(default=0, description="OPC channel (0 = broadcast to all)") - nanoleaf_token: str = Field(default="", description="Nanoleaf auth token") + nanoleaf_paired: bool = Field( + default=False, + description=( + "Whether the Nanoleaf auth token has been issued by the pairing " + "handshake. The token itself is intentionally not exposed in the " + "response -- to re-pair, delete and re-add the device." + ), + ) nanoleaf_min_interval_ms: int = Field( default=100, description="Nanoleaf client-side rate limit in ms" ) diff --git a/server/src/ledgrab/core/devices/ddp_provider.py b/server/src/ledgrab/core/devices/ddp_provider.py index e6d8ddc..045a732 100644 --- a/server/src/ledgrab/core/devices/ddp_provider.py +++ b/server/src/ledgrab/core/devices/ddp_provider.py @@ -5,6 +5,7 @@ from __future__ import annotations from typing import TYPE_CHECKING, List from ledgrab.core.devices.ddp_led_client import DDPLEDClient, parse_ddp_url +from ledgrab.utils.net_classify import validate_lan_host from ledgrab.core.devices.led_client import ( DeviceHealth, DiscoveredDevice, @@ -58,6 +59,7 @@ class DDPDeviceProvider(LEDDeviceProvider): host, port = parse_ddp_url(url) except ValueError as exc: raise ValueError(f"Invalid DDP URL: {exc}") from exc + validate_lan_host(host) logger.info("DDP device URL validated: host=%s port=%d", host, port) return {} diff --git a/server/src/ledgrab/core/devices/govee_client.py b/server/src/ledgrab/core/devices/govee_client.py index b892b7f..95ee14b 100644 --- a/server/src/ledgrab/core/devices/govee_client.py +++ b/server/src/ledgrab/core/devices/govee_client.py @@ -43,6 +43,25 @@ _DISCOVERY_REQUEST = json.dumps( ).encode("utf-8") +# Govee mandates listening on port 4002 for scan replies; the OS only lets +# one process / socket own it at a time. Serialize concurrent discover +# calls (e.g. two browser tabs hitting /discover at once) so the second +# caller waits its turn instead of getting an empty result. +_DISCOVERY_LOCK: asyncio.Lock | None = None + + +def _discovery_lock() -> asyncio.Lock: + """Return a module-level asyncio.Lock, created lazily. + + Can't construct at import-time because asyncio.Lock binds to the + running event loop and module import may precede loop creation. + """ + global _DISCOVERY_LOCK + if _DISCOVERY_LOCK is None: + _DISCOVERY_LOCK = asyncio.Lock() + return _DISCOVERY_LOCK + + def parse_govee_url(url: str) -> str: """Pull the host out of ``govee://host`` or bare ``host``.""" if not url: @@ -309,11 +328,20 @@ async def discover_govee_devices(timeout: float = 2.0) -> List[dict]: Returns a list of ``{"ip": ..., "device": ..., "sku": ..., "version": ...}`` dicts. Multicast / receive failures (no network, firewall, no LAN-enabled bulbs) yield an empty list rather than raising. + + Concurrent invocations are serialized via a module-level + ``asyncio.Lock`` so two callers don't race on the protocol-mandated + response port 4002. """ + async with _discovery_lock(): + return await _discover_govee_devices_locked(timeout) + + +async def _discover_govee_devices_locked(timeout: float) -> List[dict]: loop = asyncio.get_running_loop() - # We bind a separate socket to the response port (4002). If something - # else on the host already owns 4002 (rare; another Govee tool), the - # bind fails and we degrade gracefully to an empty result. + # We bind a separate socket to the response port (4002). The + # _DISCOVERY_LOCK serializes our own scans; the bind can still fail + # if a non-LedGrab Govee tool on the same host owns 4002. recv_sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, socket.IPPROTO_UDP) recv_sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) recv_sock.setblocking(False) @@ -324,7 +352,12 @@ async def discover_govee_devices(timeout: float = 2.0) -> List[dict]: try: recv_sock.bind(("", GOVEE_RESPONSE_PORT)) except OSError as exc: - logger.warning("Govee discovery: cannot bind %d (%s)", GOVEE_RESPONSE_PORT, exc) + logger.warning( + "Govee discovery: cannot bind port %d (%s). Another Govee " + "tool on this host may own the port; close it and retry.", + GOVEE_RESPONSE_PORT, + exc, + ) return [] send_sock.bind(("", 0)) await loop.sock_sendto( diff --git a/server/src/ledgrab/core/devices/govee_provider.py b/server/src/ledgrab/core/devices/govee_provider.py index e11f3e2..692f421 100644 --- a/server/src/ledgrab/core/devices/govee_provider.py +++ b/server/src/ledgrab/core/devices/govee_provider.py @@ -9,6 +9,7 @@ from ledgrab.core.devices.govee_client import ( discover_govee_devices, parse_govee_url, ) +from ledgrab.utils.net_classify import validate_lan_host from ledgrab.core.devices.led_client import ( DeviceHealth, DiscoveredDevice, @@ -62,6 +63,7 @@ class GoveeDeviceProvider(LEDDeviceProvider): host = parse_govee_url(url) except ValueError as exc: raise ValueError(f"Invalid Govee URL: {exc}") from exc + validate_lan_host(host) logger.info("Govee device URL validated: host=%s", host) return {} diff --git a/server/src/ledgrab/core/devices/lifx_provider.py b/server/src/ledgrab/core/devices/lifx_provider.py index bd935db..d0019e5 100644 --- a/server/src/ledgrab/core/devices/lifx_provider.py +++ b/server/src/ledgrab/core/devices/lifx_provider.py @@ -16,6 +16,7 @@ from ledgrab.core.devices.lifx_client import ( discover_lifx_bulbs, parse_lifx_url, ) +from ledgrab.utils.net_classify import validate_lan_host from ledgrab.utils import get_logger if TYPE_CHECKING: @@ -60,6 +61,7 @@ class LIFXDeviceProvider(LEDDeviceProvider): host, port = parse_lifx_url(url) except ValueError as exc: raise ValueError(f"Invalid LIFX URL: {exc}") from exc + validate_lan_host(host) logger.info("LIFX device URL validated: host=%s port=%d", host, port) return {} diff --git a/server/src/ledgrab/core/devices/nanoleaf_client.py b/server/src/ledgrab/core/devices/nanoleaf_client.py index 04a68a6..c7fe395 100644 --- a/server/src/ledgrab/core/devices/nanoleaf_client.py +++ b/server/src/ledgrab/core/devices/nanoleaf_client.py @@ -145,6 +145,12 @@ class NanoleafClient(LEDClient): def host(self) -> str: return self._host + @property + def port(self) -> int: + """Fixed at the protocol-mandated 16021. Exposed for cross-driver + symmetry with the UDP/TCP clients that also surface ``.port``.""" + return NANOLEAF_PORT + @property def is_connected(self) -> bool: return self._connected and self._http is not None diff --git a/server/src/ledgrab/core/devices/nanoleaf_provider.py b/server/src/ledgrab/core/devices/nanoleaf_provider.py index 2ab40d9..a42546d 100644 --- a/server/src/ledgrab/core/devices/nanoleaf_provider.py +++ b/server/src/ledgrab/core/devices/nanoleaf_provider.py @@ -17,6 +17,7 @@ from ledgrab.core.devices.nanoleaf_client import ( pair_nanoleaf, parse_nanoleaf_url, ) +from ledgrab.utils.net_classify import validate_lan_host from ledgrab.utils import get_logger if TYPE_CHECKING: @@ -68,6 +69,7 @@ class NanoleafDeviceProvider(LEDDeviceProvider): host = parse_nanoleaf_url(url) except ValueError as exc: raise ValueError(f"Invalid Nanoleaf URL: {exc}") from exc + validate_lan_host(host) token = await pair_nanoleaf(host) return {"nanoleaf_token": token} @@ -83,6 +85,7 @@ class NanoleafDeviceProvider(LEDDeviceProvider): host = parse_nanoleaf_url(url) except ValueError as exc: raise ValueError(f"Invalid Nanoleaf URL: {exc}") from exc + validate_lan_host(host) logger.info("Nanoleaf URL validated: host=%s", host) return {} @@ -116,6 +119,7 @@ class NanoleafDeviceProvider(LEDDeviceProvider): logger.warning("Nanoleaf discovery: zeroconf init failed (%s)", exc) return [] + browsers: list = [] try: browsers = [ AsyncServiceBrowser(aiozc.zeroconf, st, handlers=[_on_state_change]) @@ -124,9 +128,14 @@ class NanoleafDeviceProvider(LEDDeviceProvider): await asyncio.sleep(timeout) for info in discovered.values(): await info.async_request(aiozc.zeroconf, timeout=2000) - for browser in browsers: - await browser.async_cancel() finally: + # Cancel browsers in finally so a CancelledError during sleep or + # async_request still tears them down — caught by review MEDIUM #8. + for browser in browsers: + try: + await browser.async_cancel() + except (OSError, RuntimeError): + pass try: await aiozc.async_close() except (OSError, RuntimeError): diff --git a/server/src/ledgrab/core/devices/opc_provider.py b/server/src/ledgrab/core/devices/opc_provider.py index 55b8305..4e63e36 100644 --- a/server/src/ledgrab/core/devices/opc_provider.py +++ b/server/src/ledgrab/core/devices/opc_provider.py @@ -12,6 +12,7 @@ from ledgrab.core.devices.led_client import ( ProviderDeps, ) from ledgrab.core.devices.opc_client import OPCClient, parse_opc_url +from ledgrab.utils.net_classify import validate_lan_host from ledgrab.utils import get_logger if TYPE_CHECKING: @@ -53,6 +54,7 @@ class OPCDeviceProvider(LEDDeviceProvider): host, port = parse_opc_url(url) except ValueError as exc: raise ValueError(f"Invalid OPC URL: {exc}") from exc + validate_lan_host(host) logger.info("OPC device URL validated: host=%s port=%d", host, port) return {} diff --git a/server/src/ledgrab/core/devices/wiz_provider.py b/server/src/ledgrab/core/devices/wiz_provider.py index d03f81d..28058ae 100644 --- a/server/src/ledgrab/core/devices/wiz_provider.py +++ b/server/src/ledgrab/core/devices/wiz_provider.py @@ -16,6 +16,7 @@ from ledgrab.core.devices.wiz_client import ( discover_wiz_bulbs, parse_wiz_url, ) +from ledgrab.utils.net_classify import validate_lan_host from ledgrab.utils import get_logger if TYPE_CHECKING: @@ -60,6 +61,7 @@ class WiZDeviceProvider(LEDDeviceProvider): host, port = parse_wiz_url(url) except ValueError as exc: raise ValueError(f"Invalid WiZ URL: {exc}") from exc + validate_lan_host(host) logger.info("WiZ device URL validated: host=%s port=%d", host, port) return {} diff --git a/server/src/ledgrab/core/devices/yeelight_provider.py b/server/src/ledgrab/core/devices/yeelight_provider.py index fe2132f..f17fae7 100644 --- a/server/src/ledgrab/core/devices/yeelight_provider.py +++ b/server/src/ledgrab/core/devices/yeelight_provider.py @@ -17,6 +17,7 @@ from ledgrab.core.devices.yeelight_client import ( discover_yeelight_bulbs, parse_yeelight_url, ) +from ledgrab.utils.net_classify import validate_lan_host from ledgrab.utils import get_logger if TYPE_CHECKING: @@ -65,6 +66,7 @@ class YeelightDeviceProvider(LEDDeviceProvider): host = parse_yeelight_url(url) except ValueError as exc: raise ValueError(f"Invalid Yeelight URL: {exc}") from exc + validate_lan_host(host) logger.info("Yeelight device URL validated: host=%s", host) return {} diff --git a/server/src/ledgrab/static/js/features/devices.ts b/server/src/ledgrab/static/js/features/devices.ts index 7b1ff30..59b2138 100644 --- a/server/src/ledgrab/static/js/features/devices.ts +++ b/server/src/ledgrab/static/js/features/devices.ts @@ -738,7 +738,7 @@ export async function showSettings(deviceId: any) { const nmi = device.nanoleaf_min_interval_ms ?? 100; (document.getElementById('settings-nanoleaf-min-interval') as HTMLInputElement).value = String(nmi); if (nanoleafPairedGroup) { - (nanoleafPairedGroup as HTMLElement).style.display = device.nanoleaf_token ? '' : 'none'; + (nanoleafPairedGroup as HTMLElement).style.display = device.nanoleaf_paired ? '' : 'none'; } const urlLabel8 = urlGroup.querySelector('label[for="settings-device-url"]') as HTMLElement | null; const urlHint8 = urlGroup.querySelector('.input-hint') as HTMLElement | null; diff --git a/server/src/ledgrab/static/js/types.ts b/server/src/ledgrab/static/js/types.ts index 9fe76f7..1ed23b4 100644 --- a/server/src/ledgrab/static/js/types.ts +++ b/server/src/ledgrab/static/js/types.ts @@ -75,14 +75,13 @@ export interface Device { opc_channel: number; espnow_peer_mac: string; espnow_channel: number; - hue_username: string; - hue_client_key: string; + hue_paired: boolean; hue_entertainment_group_id: string; yeelight_min_interval_ms: number; wiz_min_interval_ms: number; lifx_min_interval_ms: number; govee_min_interval_ms: number; - nanoleaf_token: string; + nanoleaf_paired: boolean; nanoleaf_min_interval_ms: number; spi_speed_hz: number; spi_led_type: string; diff --git a/server/src/ledgrab/storage/device_store.py b/server/src/ledgrab/storage/device_store.py index 9243dc4..69211ea 100644 --- a/server/src/ledgrab/storage/device_store.py +++ b/server/src/ledgrab/storage/device_store.py @@ -20,8 +20,27 @@ def _enc(value: str) -> str: def _dec(value: str) -> str: - """Decrypt an envelope; legacy plaintext passes through unchanged.""" - return secret_box.decrypt(value) if value else "" + """Decrypt an envelope; legacy plaintext passes through unchanged. + + A corrupt envelope (key rotated, partial write, AEAD mismatch) returns + an empty string and logs an error rather than re-raising. Without this + fallback a single bad row would propagate up to ``BaseSqliteStore._load`` + and silently drop the entire device from ``self._items`` -- so a + regenerated ``data/.secret_key`` would make every Hue / Nanoleaf / + BLE-Govee device disappear with only a log line for a trail. Caught + by pre-merge review HIGH #5. + """ + if not value: + return "" + try: + return secret_box.decrypt(value) + except Exception as exc: + logger.error( + "Could not decrypt stored secret (%s); clearing the field. " + "Re-pair the affected device to recover.", + type(exc).__name__, + ) + return "" class Device: @@ -656,12 +675,18 @@ class DeviceStore(BaseSqliteStore[Device]): if new_name is not None and new_name != device.name: self._check_name_unique(new_name, exclude_id=device_id) - # Build new Device from existing fields + updates (immutable pattern) - device_fields = device.to_dict() - # Map 'id' back to 'device_id' for the constructor + # Build new Device from existing fields + updates (immutable pattern). + # Source kwargs directly from instance attributes via vars(), NOT + # via to_dict() -- to_dict() encrypts hue_username/hue_client_key/ + # ble_govee_key/nanoleaf_token via _enc(), and Device.__init__ + # doesn't decrypt, so a round-trip through to_dict() would leave + # the new in-memory Device holding ciphertext where plaintext + # belongs. The cached _items entry would then return ciphertext + # to provider.create_client(), and the device's runtime auth + # would silently break until server restart (caught by review). + device_fields = vars(device).copy() + # Constructor takes device_id, but attribute is id. device_fields["device_id"] = device_fields.pop("id") - # Restore datetime objects (to_dict serializes them as ISO strings) - device_fields["created_at"] = device.created_at device_fields["updated_at"] = datetime.now(timezone.utc) # Apply updates device_fields.update(updates) diff --git a/server/src/ledgrab/utils/net_classify.py b/server/src/ledgrab/utils/net_classify.py index c54370a..ba1ac78 100644 --- a/server/src/ledgrab/utils/net_classify.py +++ b/server/src/ledgrab/utils/net_classify.py @@ -134,3 +134,45 @@ def is_loopback(host: str) -> bool: if h in {"localhost", "testclient"}: return True return classify_ip(h) is HostCategory.LOOPBACK + + +_LAN_CATEGORIES: frozenset[HostCategory] = frozenset( + { + HostCategory.LOOPBACK, + HostCategory.PRIVATE, + HostCategory.LINK_LOCAL, + HostCategory.UNSPECIFIED, + } +) + + +def validate_lan_host(host: str) -> None: + """Raise ``ValueError`` when *host* is a literal public IP address. + + Used by LED-device providers to reject ``wiz://1.1.1.1:38899`` style + URLs that an authenticated (or anonymous, given the default no-auth + config) caller could otherwise weaponise into a network-scan oracle. + + Non-IP-literal hostnames (``bulb.local``, ``office-strip``) pass + through untouched -- they resolve via mDNS at connect time, where + failure to reach a real device produces the right "device unreachable" + error path anyway. The check is deliberately a syntax filter, not a + DNS-resolving SSRF guard: LedGrab is LAN-bound by design, and + aggressive DNS-time rebinding protection is the job of the platform's + network policy, not a per-driver validator. + """ + if not host: + return + h = host.strip().lower() + if h.startswith("[") and h.endswith("]"): + h = h[1:-1] + h = h.split("%", 1)[0] # strip IPv6 zone id + cat = classify_ip(h) + if cat is HostCategory.UNPARSEABLE: + return # hostname / mDNS label -- accept + if cat not in _LAN_CATEGORIES: + raise ValueError( + f"Refusing to add device at {host!r}: literal {cat.value} IP " + "addresses are rejected. LedGrab is LAN-only; if your device is " + "genuinely on a public address, use a hostname or tunnel." + ) diff --git a/server/tests/api/routes/test_devices_routes.py b/server/tests/api/routes/test_devices_routes.py index 1b59957..afcdf5f 100644 --- a/server/tests/api/routes/test_devices_routes.py +++ b/server/tests/api/routes/test_devices_routes.py @@ -340,3 +340,80 @@ class TestPairDevice: def test_missing_required_fields_returns_422(self, client): resp = client.post("/api/v1/devices/pair", json={"device_type": "nanoleaf"}) assert resp.status_code == 422 + + +class TestPairThenCreateFlow: + """End-to-end coverage: pair, then persist; assert the token is + encrypted at rest and decrypted in to_config(), and that the API + response strips the secret. + + Closes the LOW gap in the pre-merge review: pair-route tests stopped + at the 200 response, never carrying the returned fields through to + storage to verify the round-trip and the response-strip. + """ + + def test_pair_then_create_persists_encrypted_token(self, client, device_store): + from ledgrab.api.routes.devices import _device_to_response + from ledgrab.core.devices import led_client as _led_client + from ledgrab.core.devices.led_client import DeviceHealth + + class _NanoleafLikeStub: + @property + def device_type(self): + return "nanoleaf_like_stub" + + @property + def capabilities(self): + return {"manual_led_count", "requires_pairing"} + + async def pair_device(self, url): + return {"nanoleaf_token": "secret-paired-token"} + + async def validate_device(self, url): + return {"led_count": 9} + + def create_client(self, config, *, deps): + raise AssertionError("not used") + + async def check_health(self, url, http_client, prev_health=None): + return DeviceHealth(online=True) + + _led_client.register_provider(_NanoleafLikeStub()) + try: + # Step 1: pair via the route + pair_resp = client.post( + "/api/v1/devices/pair", + json={"device_type": "nanoleaf_like_stub", "url": "stub://1.2.3.4"}, + ) + assert pair_resp.status_code == 200, pair_resp.text + fields = pair_resp.json()["fields"] + assert fields == {"nanoleaf_token": "secret-paired-token"} + + # Step 2: persist via the store (skip the route's create path + # which would require a real validate_device handshake) + device = device_store.create_device( + name="E2E Paired", + url="stub://1.2.3.4", + led_count=9, + device_type="nanoleaf", + **fields, + ) + + # In-memory device holds plaintext + assert device.nanoleaf_token == "secret-paired-token" + + # to_config surfaces plaintext to the provider + config = device.to_config() + assert config.nanoleaf_token == "secret-paired-token" + + # Persisted row holds ciphertext (envelope prefix) + persisted = device.to_dict() + assert persisted["nanoleaf_token"].startswith("ENC:v1:") + assert persisted["nanoleaf_token"] != "secret-paired-token" + + # API response strips the token; only a boolean flag remains + resp = _device_to_response(device).model_dump() + assert "nanoleaf_token" not in resp + assert resp.get("nanoleaf_paired") is True + finally: + _led_client._provider_registry.pop("nanoleaf_like_stub", None) diff --git a/server/tests/test_device_store.py b/server/tests/test_device_store.py index f4561fd..360a336 100644 --- a/server/tests/test_device_store.py +++ b/server/tests/test_device_store.py @@ -270,3 +270,81 @@ def test_update_device_ignores_unknown_fields(device_store): # Should not raise even with an unknown kwarg updated = device_store.update_device(device.id, nonexistent_field="foo") assert updated.name == "Test WLED" + + +def test_update_device_preserves_plaintext_secrets(device_store): + """update_device must not corrupt encrypted-at-rest secrets in memory. + + Regression test for the to_dict() round-trip bug: to_dict() encrypts + hue_username / hue_client_key / ble_govee_key / nanoleaf_token via + _enc(), but Device.__init__ does not decrypt -- so the old + update_device path would leave the cached Device holding ciphertext + where plaintext belongs, breaking runtime auth for paired devices + until server restart. See pre-merge review CRITICAL #2. + """ + device = device_store.create_device( + name="Office panels", + url="nanoleaf://192.168.1.99", + led_count=9, + device_type="nanoleaf", + nanoleaf_token="plaintext-secret-token", + ) + assert device.nanoleaf_token == "plaintext-secret-token" + + # An innocuous rename must not touch the secret. + updated = device_store.update_device(device.id, name="Renamed panels") + assert ( + updated.nanoleaf_token == "plaintext-secret-token" + ), "rename round-tripped the secret through to_dict() and corrupted it" + + # The cached _items entry must also hold plaintext (it is what + # provider.create_client() reads). + cached = device_store.get_device(device.id) + assert cached.nanoleaf_token == "plaintext-secret-token" + + # And to_config() must surface the plaintext token to the provider. + config = cached.to_config() + assert config.nanoleaf_token == "plaintext-secret-token" + + +def test_update_device_preserves_hue_secrets(device_store): + """Same bug applies to hue_username / hue_client_key / ble_govee_key.""" + device = device_store.create_device( + name="Hue bridge", + url="http://192.168.1.50", + led_count=10, + device_type="hue", + hue_username="plain-hue-user", + hue_client_key="plain-client-key", + ) + + updated = device_store.update_device(device.id, name="Renamed bridge") + + assert updated.hue_username == "plain-hue-user" + assert updated.hue_client_key == "plain-client-key" + + +def test_from_dict_clears_corrupt_secret_envelope(device_store): + """A corrupt nanoleaf_token must not delete the device row. + + Regression test for HIGH #5: previously _dec() would raise on a bad + envelope, propagate up through BaseSqliteStore._load, and the entire + device row would silently disappear. The fix clears the offending + field and logs an error instead. + """ + payload = { + "id": "device_abc12345", + "name": "Will survive", + "url": "nanoleaf://192.168.1.50", + "led_count": 9, + "device_type": "nanoleaf", + # Looks like an envelope (ENC:v1: prefix) but the base64 body is + # truncated / corrupt -- the AES-GCM auth tag will fail to verify. + "nanoleaf_token": "ENC:v1:not-real-base64-cipher-text", + } + + restored = Device.from_dict(payload) + + # The device hydrates; the broken secret is cleared, not crashed. + assert restored.id == "device_abc12345" + assert restored.nanoleaf_token == "" diff --git a/server/tests/test_nanoleaf.py b/server/tests/test_nanoleaf.py index 793419b..4be78bc 100644 --- a/server/tests/test_nanoleaf.py +++ b/server/tests/test_nanoleaf.py @@ -322,24 +322,34 @@ def test_provider_device_type_and_capabilities(): @pytest.mark.asyncio async def test_provider_pair_device_returns_nanoleaf_token(respx_mock): - respx_mock.post(f"http://1.2.3.4:{NANOLEAF_PORT}/api/v1/new").mock( + # Use a LAN address; validate_lan_host now rejects public IPs at the + # provider boundary (review HIGH #4). + respx_mock.post(f"http://192.168.1.50:{NANOLEAF_PORT}/api/v1/new").mock( return_value=httpx.Response(200, json={"auth_token": "ttoken"}), ) provider = NanoleafDeviceProvider() - fields = await provider.pair_device("nanoleaf://1.2.3.4") + fields = await provider.pair_device("nanoleaf://192.168.1.50") assert fields == {"nanoleaf_token": "ttoken"} @pytest.mark.asyncio async def test_provider_pair_device_propagates_pairing_not_ready(respx_mock): - respx_mock.post(f"http://1.2.3.4:{NANOLEAF_PORT}/api/v1/new").mock( + respx_mock.post(f"http://192.168.1.50:{NANOLEAF_PORT}/api/v1/new").mock( return_value=httpx.Response(403), ) provider = NanoleafDeviceProvider() with pytest.raises(PairingNotReady): - await provider.pair_device("nanoleaf://1.2.3.4") + await provider.pair_device("nanoleaf://192.168.1.50") + + +@pytest.mark.asyncio +async def test_provider_pair_device_rejects_public_ip(): + """validate_lan_host fires before pair_nanoleaf even runs.""" + provider = NanoleafDeviceProvider() + with pytest.raises(ValueError, match="LedGrab is LAN-only"): + await provider.pair_device("nanoleaf://1.1.1.1") @pytest.mark.asyncio diff --git a/server/tests/test_net_classify.py b/server/tests/test_net_classify.py index cd26326..2609c4d 100644 --- a/server/tests/test_net_classify.py +++ b/server/tests/test_net_classify.py @@ -135,3 +135,55 @@ def test_is_loopback_recognises_loopback(host: str) -> None: ) def test_is_loopback_rejects_other(host) -> None: assert is_loopback(host) is False + + +# ─── validate_lan_host ──────────────────────────────────────────── + + +from ledgrab.utils.net_classify import validate_lan_host # noqa: E402 + + +@pytest.mark.parametrize( + "host", + [ + "127.0.0.1", + "10.0.0.5", + "192.168.1.50", + "172.16.0.10", + "fe80::1", + "fc00::1", + "0.0.0.0", + # IPv6 with brackets and zone id + "[fe80::1%eth0]", + # Plain hostnames pass through (mDNS / bare-label) + "bulb.local", + "office-strip", + "controller.lan", + "", # empty also passes (caller handles validation upstream) + ], +) +def test_validate_lan_host_accepts_local(host) -> None: + """Loopback / private / link-local / hostnames must not raise.""" + validate_lan_host(host) + + +@pytest.mark.parametrize( + "host", + [ + "1.1.1.1", # Cloudflare DNS (routable public) + "8.8.8.8", # Google DNS + "224.0.0.1", # multicast — not a LAN device address + "2606:4700:4700::1111", # Cloudflare DNS IPv6 + ], +) +def test_validate_lan_host_rejects_public_or_multicast(host) -> None: + """Reject genuinely-public addresses and multicast targets. + + Note: Python's :func:`ipaddress.ip_address(...).is_private` treats + RFC6890 ranges (documentation 192.0.2/24, former class E 240/4, etc.) + as private, so those are accepted as "local-ish" by our classifier -- + which is the correct policy for LedGrab: those ranges aren't routable + public addresses, and refusing them would be unhelpful theatre. + """ + with pytest.raises(ValueError, match="LedGrab is LAN-only"): + validate_lan_host(host)