Ignore malformed mDNS service names in browser callbacks

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)

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:

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)

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."""

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(

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"]

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.