diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index 08e3e817..4f1a24f9 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -29,7 +29,7 @@ import tempfile import threading import time -from typing import Any +from typing import Any, TypeGuard from zeroconf import ServiceInfo from zeroconf.asyncio import AsyncZeroconf @@ -45,6 +45,7 @@ from pyhap.const import ( HAP_PERMISSION_NOTIFY, HAP_PERMISSION_READ, + HAP_PERMISSION_TIMED_WRITE, HAP_PERMISSION_WRITE, HAP_PERMISSION_WRITE_RESPONSE, HAP_PROTOCOL_SHORT_VERSION, @@ -93,6 +94,11 @@ MAX_PREPARED_WRITE_TTL_SECONDS = 30.0 +def _is_hap_int(value: Any) -> TypeGuard[int]: + """Whether ``value`` is a JSON integer, not a bool.""" + return isinstance(value, int) and not isinstance(value, bool) + + def _stored_value(char: Characteristic, value: HAPValue) -> HAPValue: """Return the value ``client_update_value`` stores for ``value``.""" try: @@ -247,6 +253,7 @@ def __init__( async_zeroconf_instance: Any = None, zeroconf_server: Any = None, advertise: bool = True, + enforce_timed_writes: bool = False, ) -> None: """Initialize a new AccessoryDriver object. @@ -301,6 +308,13 @@ def __init__( the driver starts. Pass False to run the HAP server without mDNS advertising, e.g. for tests or benchmarks. :type advertise: bool + + :param enforce_timed_writes: Whether to reject a plain write to a + characteristic with the timed write ("tw") permission with -70410, + as the HAP specification requires. Off by default, because Home + hubs write some tw characteristics, e.g. HomeKit Camera Active, + without a timed write. + :type enforce_timed_writes: bool """ if loop is None: if sys.platform == "win32": @@ -333,6 +347,7 @@ def __init__( self.stop_event = threading.Event() self.safe_mode = False + self.enforce_timed_writes = enforce_timed_writes self.mdns_service_info: ServiceInfo | None = None self._srp_verifier: SrpServer | None = None @@ -1060,6 +1075,23 @@ def set_characteristics( """ queries: list[dict[str, Any]] = chars_query[HAP_REPR_CHARS] + timed_write = HAP_REPR_PID in chars_query + if timed_write and not self._take_prepared_write( + chars_query[HAP_REPR_PID], client_addr + ): + # The spec requires a timed write that was never prepared, was + # already used or has expired to be ignored as a whole. + return { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: query[HAP_REPR_AID], + HAP_REPR_IID: query[HAP_REPR_IID], + HAP_REPR_STATUS: HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST, + } + for query in queries + ] + } + not_notifiable = self._notify(queries, client_addr) updates_by_accessories_services: dict[ @@ -1070,12 +1102,6 @@ def set_characteristics( ) char_to_iid: dict[Characteristic, int] = {} - expired = False - if HAP_REPR_PID in chars_query: - pid = chars_query[HAP_REPR_PID] - expire_time = self.prepared_writes.get(client_addr, {}).pop(pid, None) - expired = expire_time is None or time.time() > expire_time - primary_accessory = self.accessory assert primary_accessory is not None # noqa: S101 primary_aid = primary_accessory.aid @@ -1090,7 +1116,7 @@ def set_characteristics( iid = query[HAP_REPR_IID] if (aid, iid) in not_notifiable: continue - if HAP_REPR_VALUE not in query and not expired: + if HAP_REPR_VALUE not in query: # Listed with status 0 if the response has other results. results[aid][iid] = {HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS} continue @@ -1120,6 +1146,22 @@ def set_characteristics( } continue + if ( + value is not None + and not timed_write + and HAP_PERMISSION_TIMED_WRITE in char.properties[PROP_PERMISSIONS] + ): + if self.enforce_timed_writes: + results[aid][iid] = { + HAP_REPR_STATUS: HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST + } + continue + logger.debug( + "%s: %s requires a timed write but was written without one", + client_addr, + char.display_name, + ) + # Only characteristics with the write response permission answer # a write with a value, and only when the controller asks for it. write_response = ( @@ -1208,6 +1250,14 @@ def set_characteristics( return {HAP_REPR_CHARS: characteristics} if nonempty_results_exist else None + def _take_prepared_write(self, pid: Any, client_addr: tuple[str, int]) -> bool: + """Consume the prepared write ``pid`` and return whether it is still valid.""" + if not _is_hap_int(pid): + # Controller input. A list or dict is not even hashable. + return False + expires_at = self.prepared_writes.get(client_addr, {}).pop(pid, None) + return expires_at is not None and time.monotonic() <= expires_at + def _store_write_response( self, char: Characteristic, @@ -1258,10 +1308,14 @@ def prepare( :type prepare_query: dict """ + if not isinstance(prepare_query, dict): + return {HAP_REPR_STATUS: HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST} + ttl = prepare_query.get(HAP_REPR_TTL) + pid = prepare_query.get(HAP_REPR_PID) + if not (_is_hap_int(ttl) and _is_hap_int(pid)): + return {HAP_REPR_STATUS: HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST} try: - ttl = prepare_query[HAP_REPR_TTL] - pid = prepare_query[HAP_REPR_PID] - now = time.time() + now = time.monotonic() pending = self.prepared_writes.setdefault(client_addr, {}) # Prune expired entries and cap the number of pending writes so a # paired client cannot grow prepared_writes without bound by @@ -1269,13 +1323,13 @@ def prepare( for stale_pid in [ prepared_pid for prepared_pid, expires_at in pending.items() - if expires_at <= now + if expires_at < now ]: del pending[stale_pid] if len(pending) >= MAX_PREPARED_WRITES_PER_CLIENT: del pending[min(pending, key=pending.get)] pending[pid] = now + min(ttl / 1000, MAX_PREPARED_WRITE_TTL_SECONDS) - except (KeyError, TypeError, ValueError): + except OverflowError: return {HAP_REPR_STATUS: HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST} return {HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS} diff --git a/pyhap/const.py b/pyhap/const.py index 2f70f950..aa9468ab 100644 --- a/pyhap/const.py +++ b/pyhap/const.py @@ -56,6 +56,7 @@ HAP_PERMISSION_HIDDEN = "hd" HAP_PERMISSION_NOTIFY = "ev" HAP_PERMISSION_READ = "pr" +HAP_PERMISSION_TIMED_WRITE = "tw" HAP_PERMISSION_WRITE = "pw" HAP_PERMISSION_WRITE_RESPONSE = "wr" diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index 13a5d471..63caf360 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -1,7 +1,6 @@ """Tests for pyhap.accessory_driver.""" import asyncio -from concurrent.futures import ThreadPoolExecutor from functools import partial import os import tempfile @@ -1280,24 +1279,33 @@ def fail_callback(*_: Any) -> NoReturn: } -def test_start_from_sync(driver: AccessoryDriver) -> None: - """Start from sync.""" +def test_start_from_sync(async_zeroconf: Any) -> None: + """Start from sync, on the loop and executor the driver makes itself. - class Acc(Accessory): - @Accessory.run_at_interval(0) - async def run(self) -> None: - driver.executor = ThreadPoolExecutor() - driver.loop.set_default_executor(driver.executor) - await driver.async_stop() + Not the shared test loop: stopping shuts the executor down, which would + leave that loop unable to run later tests' executor jobs. + """ + with ( + patch("pyhap.accessory_driver.HAPServer.async_stop", new_callable=MagicMock), + patch("pyhap.accessory_driver.HAPServer.async_start", new_callable=AsyncMock), + patch("pyhap.accessory_driver.AccessoryDriver.persist"), + ): + driver = AccessoryDriver() + + class Acc(Accessory): + @Accessory.run_at_interval(0) + async def run(self) -> None: + await driver.async_stop() - def setup_message(self) -> None: - pass + def setup_message(self) -> None: + pass - acc = Acc(driver, "TestAcc") - driver.add_accessory(acc) - with patch.object(driver.loop, "close"): + acc = Acc(driver, "TestAcc") + driver.add_accessory(acc) driver.start() + assert driver.loop.is_closed() + def test_accessory_level_callbacks(driver: AccessoryDriver) -> None: bridge = Bridge(driver, "mybridge") @@ -2084,7 +2092,7 @@ def test_prepare_bounds_prepared_writes(driver: AccessoryDriver) -> None: # An absurd TTL is clamped to the maximum. driver.prepare({HAP_REPR_TTL: 10_000_000_000, HAP_REPR_PID: 1}, client_addr) assert ( - driver.prepared_writes[client_addr][1] - time.time() + driver.prepared_writes[client_addr][1] - time.monotonic() <= MAX_PREPARED_WRITE_TTL_SECONDS ) diff --git a/tests/test_timed_write.py b/tests/test_timed_write.py new file mode 100644 index 00000000..929c919a --- /dev/null +++ b/tests/test_timed_write.py @@ -0,0 +1,309 @@ +"""Tests for timed writes (prepare, then write with the pid).""" + +from collections.abc import Iterator +import logging +from typing import Any, NamedTuple +from unittest.mock import MagicMock, patch + +import pytest + +from pyhap.accessory import Accessory, get_topic +from pyhap.accessory_driver import AccessoryDriver +from pyhap.const import ( + HAP_PERMISSION_NOTIFY, + HAP_REPR_AID, + HAP_REPR_CHARS, + HAP_REPR_IID, + HAP_REPR_PID, + HAP_REPR_STATUS, + HAP_REPR_TTL, + HAP_REPR_VALUE, + HAP_SERVER_STATUS, +) +from pyhap.service import Service + +CLIENT = ("1.2.3.4", 5000) +NOW = 1000.0 +REJECTED = HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST + + +class WritableChar(NamedTuple): + """A characteristic of the test accessory and its setter.""" + + setter: MagicMock + aid: int + iid: int + + +def _writable(acc: Accessory, service: Service, name: str) -> WritableChar: + """Give the char ``name`` of ``service`` a mock setter.""" + setter = MagicMock() + char = service.configure_char(name, setter_callback=setter) + iid = acc.iid_manager.get_iid(char) + assert acc.aid is not None + assert iid is not None + return WritableChar(setter, acc.aid, iid) + + +@pytest.fixture +def chars(driver: AccessoryDriver) -> Iterator[dict[str, WritableChar]]: + """Add a garage door opener with a camera operating mode service. + + Target Door State is a plain write characteristic that iOS still writes + with timed writes, HomeKit Camera Active has the timed write permission. + """ + acc = Accessory(driver, "Garage") + door = driver.loader.get_service("GarageDoorOpener") + camera = driver.loader.get_service("CameraOperatingMode") + acc.add_service(door, camera) + driver.add_accessory(acc) + with patch.object(driver, "publish"): + yield { + "TargetDoorState": _writable(acc, door, "TargetDoorState"), + "HomeKitCameraActive": _writable(acc, camera, "HomeKitCameraActive"), + } + + +@pytest.fixture +def door(chars: dict[str, WritableChar]) -> WritableChar: + """Return the Target Door State of the test accessory.""" + return chars["TargetDoorState"] + + +def _write(aid: int, iid: int, pid: int) -> dict[str, Any]: + """Return a timed write of value 1 that also subscribes to the char.""" + return { + HAP_REPR_PID: pid, + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: aid, + HAP_REPR_IID: iid, + HAP_REPR_VALUE: 1, + HAP_PERMISSION_NOTIFY: True, + } + ], + } + + +def test_prepared_write_is_applied( + driver: AccessoryDriver, + door: WritableChar, +) -> None: + """Test a write with a prepared pid inside its TTL is applied.""" + with patch("pyhap.accessory_driver.time.monotonic", return_value=NOW) as now: + driver.prepare({HAP_REPR_TTL: 2500, HAP_REPR_PID: 7}, CLIENT) + now.return_value = NOW + 2.5 + assert driver.set_characteristics(_write(door.aid, door.iid, 7), CLIENT) is None + + door.setter.assert_called_once_with(1) + assert CLIENT in driver.topics[get_topic(door.aid, door.iid)] + + +@pytest.mark.parametrize( + ("write_pid", "delay"), + [(8, 0.0), (1, 2.6), ([1], 0.0), (True, 0.0)], + ids=["unknown_pid", "expired_pid", "unhashable_pid", "bool_pid"], +) +def test_unprepared_write_is_ignored( + driver: AccessoryDriver, + door: WritableChar, + write_pid: Any, + delay: float, +) -> None: + """Test a write with an unknown or expired pid is ignored as a whole.""" + with patch("pyhap.accessory_driver.time.monotonic", return_value=NOW) as now: + driver.prepare({HAP_REPR_TTL: 2500, HAP_REPR_PID: 1}, CLIENT) + now.return_value = NOW + delay + response = driver.set_characteristics( + _write(door.aid, door.iid, write_pid), CLIENT + ) + + assert response == { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: door.aid, + HAP_REPR_IID: door.iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST, + } + ] + } + door.setter.assert_not_called() + assert get_topic(door.aid, door.iid) not in driver.topics + + +def test_prepared_write_is_kept_until_its_ttl_ends( + driver: AccessoryDriver, + door: WritableChar, +) -> None: + """Test a later prepare at the last moment of a TTL keeps that write.""" + with patch("pyhap.accessory_driver.time.monotonic", return_value=NOW) as now: + driver.prepare({HAP_REPR_TTL: 2500, HAP_REPR_PID: 7}, CLIENT) + now.return_value = NOW + 2.5 + driver.prepare({HAP_REPR_TTL: 2500, HAP_REPR_PID: 8}, CLIENT) + assert driver.set_characteristics(_write(door.aid, door.iid, 7), CLIENT) is None + + door.setter.assert_called_once_with(1) + + +def test_unprepared_write_ignores_every_characteristic( + driver: AccessoryDriver, + chars: dict[str, WritableChar], +) -> None: + """Test a write with an unknown pid writes none of its characteristics.""" + door, camera = chars["TargetDoorState"], chars["HomeKitCameraActive"] + query = { + HAP_REPR_PID: 8, + HAP_REPR_CHARS: [ + {HAP_REPR_AID: door.aid, HAP_REPR_IID: door.iid, HAP_REPR_VALUE: 1}, + {HAP_REPR_AID: camera.aid, HAP_REPR_IID: camera.iid, HAP_REPR_VALUE: 0}, + ], + } + + response = driver.set_characteristics(query, CLIENT) + + assert _statuses(response) == [REJECTED, REJECTED] + door.setter.assert_not_called() + camera.setter.assert_not_called() + + +@pytest.mark.parametrize( + "prepare_query", + [ + {HAP_REPR_TTL: 2500, HAP_REPR_PID: "7"}, + {HAP_REPR_TTL: 2500, HAP_REPR_PID: 1.5}, + {HAP_REPR_TTL: 2500, HAP_REPR_PID: True}, + {HAP_REPR_TTL: "2500", HAP_REPR_PID: 7}, + {HAP_REPR_TTL: 2500.0, HAP_REPR_PID: 7}, + {HAP_REPR_TTL: 10**400, HAP_REPR_PID: 7}, + {HAP_REPR_PID: 7}, + {HAP_REPR_TTL: 2500}, + [2500, 7], + None, + ], + ids=[ + "str_pid", + "float_pid", + "bool_pid", + "str_ttl", + "float_ttl", + "overflowing_ttl", + "missing_ttl", + "missing_pid", + "list", + "null", + ], +) +def test_prepare_refuses_non_integers( + driver: AccessoryDriver, prepare_query: Any +) -> None: + """Test a prepare that is no object or lacks an integer ttl or pid is refused.""" + assert driver.prepare(prepare_query, CLIENT) == {HAP_REPR_STATUS: REJECTED} + assert driver.prepared_writes.get(CLIENT, {}) == {} + + +def test_prepared_pid_is_used_once( + driver: AccessoryDriver, + door: WritableChar, +) -> None: + """Test a prepared pid cannot be replayed.""" + driver.prepare({HAP_REPR_TTL: 2500, HAP_REPR_PID: 7}, CLIENT) + assert driver.set_characteristics(_write(door.aid, door.iid, 7), CLIENT) is None + + response = driver.set_characteristics(_write(door.aid, door.iid, 7), CLIENT) + + assert response is not None + assert response[HAP_REPR_CHARS][0][HAP_REPR_STATUS] == ( + HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST + ) + door.setter.assert_called_once_with(1) + + +def test_prepared_pid_belongs_to_its_connection( + driver: AccessoryDriver, + door: WritableChar, +) -> None: + """Test a pid prepared on one connection is unknown on another.""" + driver.prepare({HAP_REPR_TTL: 2500, HAP_REPR_PID: 7}, CLIENT) + + response = driver.set_characteristics( + _write(door.aid, door.iid, 7), ("1.2.3.4", 5001) + ) + + assert response is not None + assert response[HAP_REPR_CHARS][0][HAP_REPR_STATUS] == ( + HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST + ) + door.setter.assert_not_called() + + +def _statuses(response: dict[str, Any] | None) -> list[int]: + """Return the statuses of a write response, none for a plain success.""" + return [char[HAP_REPR_STATUS] for char in (response or {}).get(HAP_REPR_CHARS, [])] + + +def _highest_level(records: list[logging.LogRecord]) -> int: + """Return the highest log level in ``records``, 0 without any.""" + return max((record.levelno for record in records), default=0) + + +@pytest.mark.parametrize( + ("enforce", "name", "request_extra", "value", "statuses", "calls", "logged"), + [ + (True, "HomeKitCameraActive", {}, 0, [REJECTED], 0, 0), + (True, "HomeKitCameraActive", {HAP_REPR_PID: 7}, 0, [], 1, 0), + (True, "HomeKitCameraActive", {}, None, [REJECTED], 0, 0), + (True, "TargetDoorState", {}, 0, [], 1, 0), + (False, "HomeKitCameraActive", {}, 0, [], 1, 1), + (False, "HomeKitCameraActive", {}, None, [REJECTED], 0, 0), + (False, "HomeKitCameraActive", {HAP_REPR_PID: 7}, 0, [], 1, 0), + (False, "TargetDoorState", {}, 0, [], 1, 0), + ], + ids=[ + "enforced_plain_write", + "enforced_timed_write", + "enforced_null_value", + "enforced_plain_write_without_tw", + "lenient_plain_write", + "lenient_null_value", + "lenient_timed_write", + "lenient_plain_write_without_tw", + ], +) +def test_timed_write_permission( + driver: AccessoryDriver, + chars: dict[str, WritableChar], + caplog: pytest.LogCaptureFixture, + enforce: bool, + name: str, + request_extra: dict[str, Any], + value: int | None, + statuses: list[int], + calls: int, + logged: int, +) -> None: + """Test a plain write to a timed write characteristic is refused or logged.""" + caplog.set_level(logging.DEBUG, logger="pyhap.accessory_driver") + char = chars[name] + driver.enforce_timed_writes = enforce + driver.prepare({HAP_REPR_TTL: 2500, HAP_REPR_PID: 7}, CLIENT) + query = { + HAP_REPR_CHARS: [ + {HAP_REPR_AID: char.aid, HAP_REPR_IID: char.iid, HAP_REPR_VALUE: value} + ], + **request_extra, + } + + response = driver.set_characteristics(query, CLIENT) + + assert _statuses(response) == statuses + assert char.setter.call_count == calls + assert caplog.text.count("requires a timed write") == logged + # Lenient mode must not warn, hubs write these values all the time. + assert _highest_level(caplog.records) < logging.WARNING + + +def test_enforce_timed_writes_defaults_to_off(driver: AccessoryDriver) -> None: + """Test timed writes are only enforced on request.""" + assert driver.enforce_timed_writes is False + enforced = AccessoryDriver(loop=driver.loop, enforce_timed_writes=True) + assert enforced.enforce_timed_writes is True