diff --git a/esphome_device_builder/controllers/_device_state_monitor/mdns.py b/esphome_device_builder/controllers/_device_state_monitor/mdns.py index b41deb2dc..3a89d32e9 100644 --- a/esphome_device_builder/controllers/_device_state_monitor/mdns.py +++ b/esphome_device_builder/controllers/_device_state_monitor/mdns.py @@ -29,7 +29,7 @@ from zeroconf.const import _CLASS_IN, _TYPE_A, _TYPE_AAAA, _TYPE_SRV, _TYPE_TXT from ...helpers.async_ import create_logged_task, drain_tasks -from ...helpers.hostname import normalize_hostname +from ...helpers.hostname import normalize_hostname, valid_mdns_service_name from ...helpers.ip import drop_unusable_addresses from ...models import DeviceState from .._reachability_tracker import MdnsCacheInfo @@ -495,6 +495,8 @@ def _on_browser_event( # inner handler only sees the events it cares about, # letting the upstream ``DashboardImportDiscovery`` # piggy-back on the same dispatch path. + if not valid_mdns_service_name(name): + return importable = self._monitor.importable if service_type == _ESPHOME_SERVICE_TYPE: self._on_esphomelib_service_state_change(zeroconf, service_type, name, state_change) diff --git a/esphome_device_builder/controllers/remote_build/discovery.py b/esphome_device_builder/controllers/remote_build/discovery.py index f4a3367f6..1165b35ef 100644 --- a/esphome_device_builder/controllers/remote_build/discovery.py +++ b/esphome_device_builder/controllers/remote_build/discovery.py @@ -21,6 +21,7 @@ from zeroconf.asyncio import AsyncServiceBrowser, AsyncServiceInfo from ...helpers.dashboard_advertise import SERVICE_TYPE +from ...helpers.hostname import valid_mdns_service_name from ...models import EventType, RemoteBuildHostRemovedData from ._mdns import endpoints_equal, peer_from_service_info @@ -99,6 +100,8 @@ def on_service_state_change( :meth:`_resolve_and_apply` once the SRV / TXT round-trip completes). """ + if not valid_mdns_service_name(name): + return if name == controller.state.own_instance_name: return if state_change == ServiceStateChange.Removed: diff --git a/esphome_device_builder/discover.py b/esphome_device_builder/discover.py index be42fc672..5d63d2ad2 100644 --- a/esphome_device_builder/discover.py +++ b/esphome_device_builder/discover.py @@ -29,6 +29,7 @@ from zeroconf.asyncio import AsyncServiceBrowser, AsyncServiceInfo, AsyncZeroconf from .helpers.dashboard_advertise import SERVICE_TYPE +from .helpers.hostname import valid_mdns_service_name _FORMAT = "{: <7}|{: <24}|{: <21}|{: <18}|{: <16}|{: <12}|{: <16}" _COLUMN_NAMES = ( @@ -198,9 +199,11 @@ def _on_service_state_change( :mod:`controllers._device_state_monitor` / :mod:`controllers.remote_build.controller`). """ - # The mDNS service name is peer-controlled; sanitize before printing so a - # hostile broadcaster can't inject ANSI escapes / newlines / null bytes - # into the terminal via the instance label. + if not valid_mdns_service_name(name): + return + # Control characters never get past the guard above, but the instance + # label is still peer-controlled Unicode; strip the rest of the + # non-printables and length-cap before it reaches the terminal. short_name = _safe_label(name.partition(".")[0], _MAX_NAME_DISPLAY) state = "OFFLINE" if state_change is ServiceStateChange.Removed else "ONLINE" info = AsyncServiceInfo(service_type, name) diff --git a/esphome_device_builder/helpers/hostname.py b/esphome_device_builder/helpers/hostname.py index b15bbf199..4dc39e4cb 100644 --- a/esphome_device_builder/helpers/hostname.py +++ b/esphome_device_builder/helpers/hostname.py @@ -2,6 +2,29 @@ from __future__ import annotations +import logging +from functools import lru_cache + +from zeroconf import BadTypeInNameException, service_type_name + +_LOGGER = logging.getLogger(__name__) + + +@lru_cache(maxsize=256) +def valid_mdns_service_name(name: str) -> bool: + """ + Return True when *name* would construct a ``ServiceInfo`` without raising. + + The browser hands callbacks raw wire names; gate on this before + building a ``ServiceInfo`` from one (#2620). + """ + try: + service_type_name(name, strict=False) + except BadTypeInNameException as err: + _LOGGER.debug("Ignoring invalid mDNS service name %r: %s", name, err) + return False + return True + def default_mdns_address(name: str) -> str: """Return the mDNS address ESPHome derives from a device *name* by default.""" diff --git a/tests/test_discover.py b/tests/test_discover.py index b5adec586..6355f7a4d 100644 --- a/tests/test_discover.py +++ b/tests/test_discover.py @@ -130,26 +130,18 @@ def test_per_column_caps_match_format_widths() -> None: assert _MAX_PIN_DISPLAY == 64 -def test_on_service_state_change_sanitizes_hostile_service_name( +def test_on_service_state_change_drops_hostile_service_name( capsys: pytest.CaptureFixture[str], ) -> None: - """ESC bytes in the mDNS instance name don't reach stdout.""" - fake_info = MagicMock() - fake_info.properties = {} - fake_info.ip_addresses_by_version.return_value = ["192.168.1.10"] - fake_info.port = 6052 - - with patch("esphome_device_builder.discover.AsyncServiceInfo", return_value=fake_info): - _on_service_state_change( - MagicMock(), - "_esphomebuilder._tcp.local.", - "\x1b[2Jevil._esphomebuilder._tcp.local.", - ServiceStateChange.Added, - ) - - captured = capsys.readouterr().out - assert "\x1b" not in captured - assert "[2Jevil" in captured + """A control-character instance name prints no row at all (#2620).""" + _on_service_state_change( + MagicMock(), + "_esphomebuilder._tcp.local.", + "\x1b[2Jevil._esphomebuilder._tcp.local.", + ServiceStateChange.Added, + ) + + assert capsys.readouterr().out == "" def test_on_service_state_change_sanitizes_hostile_txt_values( diff --git a/tests/test_mdns_malformed_names.py b/tests/test_mdns_malformed_names.py new file mode 100644 index 000000000..c615a056f --- /dev/null +++ b/tests/test_mdns_malformed_names.py @@ -0,0 +1,83 @@ +"""Malformed mDNS service-instance names are dropped before any handler runs (#2620).""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +import pytest +from zeroconf import ServiceStateChange + +from esphome_device_builder.controllers._device_state_monitor import DeviceStateMonitor +from esphome_device_builder.controllers._device_state_monitor._state import MonitorState +from esphome_device_builder.controllers._device_state_monitor.importable import ImportableDiscovery +from esphome_device_builder.controllers._device_state_monitor.mdns import MdnsSource +from esphome_device_builder.helpers.hostname import valid_mdns_service_name + +_ESPHOME = "_esphomelib._tcp.local." +_HTTP = "_http._tcp.local." + +# The reporter's IRIS alarm module: control characters in the instance label. +_BAD_HTTP = f"IRIS\x10µA \x10½pG.{_HTTP}" +_BAD_ESPHOME = f"IRIS\x10evil.{_ESPHOME}" + +_ALL_CHANGES = ( + ServiceStateChange.Added, + ServiceStateChange.Updated, + ServiceStateChange.Removed, +) + + +def _make_monitor() -> DeviceStateMonitor: + monitor = DeviceStateMonitor.__new__(DeviceStateMonitor) + monitor.state = MonitorState() + monitor.importable = ImportableDiscovery(monitor) + monitor.importable.setup() + monitor.mdns = MdnsSource(monitor) + monitor._tasks = set() + monitor._get_devices_by_name = lambda _name: [] + monitor._find_device_by_name = lambda _name: None + return monitor + + +def test_valid_mdns_service_name() -> None: + assert valid_mdns_service_name(f"klo.{_ESPHOME}") + assert valid_mdns_service_name(f"My Printer (2).{_HTTP}") + assert not valid_mdns_service_name(_BAD_HTTP) + assert not valid_mdns_service_name(_BAD_ESPHOME) + assert not valid_mdns_service_name(f"{'x' * 64}.{_HTTP}") + + +@pytest.mark.parametrize("state_change", _ALL_CHANGES) +@pytest.mark.parametrize(("service_type", "name"), [(_HTTP, _BAD_HTTP), (_ESPHOME, _BAD_ESPHOME)]) +def test_browser_dispatch_drops_malformed_name( + service_type: str, name: str, state_change: ServiceStateChange +) -> None: + """The #2620 crash path: a raw wire name zeroconf's ServiceInfo rejects.""" + monitor = _make_monitor() + + monitor.mdns._on_browser_event(MagicMock(), service_type, name, state_change) + + assert not monitor._tasks + assert not monitor.state.http_urls + assert not monitor.importable._import_discovery.import_state + + +def test_browser_dispatch_guard_precedes_handlers(monkeypatch: pytest.MonkeyPatch) -> None: + monitor = _make_monitor() + calls: list[str] = [] + monkeypatch.setattr( + monitor.mdns, + "_on_http_service_state_change", + lambda *_a: calls.append("monitor"), + ) + monkeypatch.setattr( + monitor.importable, + "on_http_service_state_change", + lambda *_a: calls.append("importable"), + ) + + monitor.mdns._on_browser_event(MagicMock(), _HTTP, _BAD_HTTP, ServiceStateChange.Added) + assert calls == [] + + monitor.mdns._on_browser_event(MagicMock(), _HTTP, f"klo.{_HTTP}", ServiceStateChange.Added) + assert calls == ["monitor", "importable"] diff --git a/tests/test_remote_build_controller.py b/tests/test_remote_build_controller.py index 7ac303a53..7bd99222f 100644 --- a/tests/test_remote_build_controller.py +++ b/tests/test_remote_build_controller.py @@ -280,6 +280,15 @@ def test_on_service_state_change_filters_own_advertise(tmp_path: Path) -> None: assert controller.offloader.state.peers == {} +def test_on_service_state_change_drops_malformed_name(tmp_path: Path) -> None: + """A wire name zeroconf's ServiceInfo rejects is ignored instead of raising (#2620).""" + controller = _make_controller(config_dir=tmp_path) + controller.offloader._on_service_state_change( + MagicMock(), SERVICE_TYPE, f"IRIS\x10evil.{SERVICE_TYPE}", ServiceStateChange.Added + ) + assert controller.offloader.state.peers == {} + + def test_is_self_endpoint_matches_advertised_host_and_port(tmp_path: Path) -> None: """A ``(host, port)`` matching the advertiser's published endpoint reports True.