From 787a52518b1c1f99b5ffc80bc9170f2da645baa9 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Sun, 8 Nov 2026 19:25:31 +0100 Subject: [PATCH 01/21] Return -70405 when reading a characteristic without paired read GET /characteristics returned the stored value of every characteristic, including write-only ones such as Identify or a camera key. The HAP spec only allows reads of characteristics with the paired read (pr) permission and answers everything else with -70405. HAP-NodeJS does the same. The getter is not called for such characteristics. The /accessories database already omitted their value; this adds tests for it. --- pyhap/accessory_driver.py | 17 ++++-- tests/test_accessory_driver.py | 105 +++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 5 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index a68eae6c..bdbccad0 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -36,9 +36,10 @@ from pyhap import util from pyhap.accessory import Accessory, Bridge, get_topic -from pyhap.characteristic import Characteristic, CharacteristicError +from pyhap.characteristic import PROP_PERMISSIONS, Characteristic, CharacteristicError from pyhap.const import ( HAP_PERMISSION_NOTIFY, + HAP_PERMISSION_READ, HAP_PROTOCOL_SHORT_VERSION, HAP_REPR_ACCS, HAP_REPR_AID, @@ -955,10 +956,16 @@ def get_characteristics(self, char_ids: Any) -> dict[str, Any]: available = False # A characteristic IID may resolve to a Service or be unknown; - # only a Characteristic can report a value. - if available and isinstance(char, Characteristic): - rep[HAP_REPR_VALUE] = char.get_value() - rep[HAP_REPR_STATUS] = HAP_SERVER_STATUS.SUCCESS + # only a Characteristic can report a value, and only when it + # has the paired read permission. + if isinstance(char, Characteristic): + if HAP_PERMISSION_READ not in char.properties[PROP_PERMISSIONS]: + rep[HAP_REPR_STATUS] = ( + HAP_SERVER_STATUS.WRITE_ONLY_CHARACTERISTIC + ) + elif available: + rep[HAP_REPR_VALUE] = char.get_value() + rep[HAP_REPR_STATUS] = HAP_SERVER_STATUS.SUCCESS except CharacteristicError: logger.error( "%s: Error getting value for characteristic %s.", diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index 97627156..d4ebbe8f 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -32,10 +32,12 @@ ) from pyhap.const import ( HAP_PERMISSIONS, + HAP_REPR_ACCS, HAP_REPR_AID, HAP_REPR_CHARS, HAP_REPR_IID, HAP_REPR_PID, + HAP_REPR_SERVICES, HAP_REPR_STATUS, HAP_REPR_TTL, HAP_REPR_VALUE, @@ -140,6 +142,109 @@ def test_get_characteristics_honors_availability( getter.assert_called_once_with() +def _add_bridged_or_standalone( + driver: AccessoryDriver, permissions: list[str], bridged: bool +) -> tuple[Accessory, Characteristic, MagicMock]: + """Add an accessory with one int characteristic that has a getter.""" + acc = Accessory(driver, "Test Accessory", aid=2 if bridged else STANDALONE_AID) + service = Service(uuid1(), "Test Service") + char = Characteristic( + "Value", + uuid1(), + {PROP_FORMAT: HAP_FORMAT_INT, PROP_PERMISSIONS: permissions}, + ) + getter = MagicMock(return_value=42) + char.getter_callback = getter + service.add_characteristic(char) + acc.add_service(service) + if bridged: + bridge = Bridge(driver, "Test Bridge") + bridge.add_accessory(acc) + driver.add_accessory(bridge) + else: + driver.add_accessory(acc) + return acc, char, getter + + +READ_PERMISSION_CASES = [ + pytest.param( + ["pr"], + {HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS, HAP_REPR_VALUE: 42}, + 1, + id="read", + ), + pytest.param( + ["pr", "pw", "ev"], + {HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS, HAP_REPR_VALUE: 42}, + 1, + id="read-write-notify", + ), + pytest.param( + ["pw"], + {HAP_REPR_STATUS: HAP_SERVER_STATUS.WRITE_ONLY_CHARACTERISTIC}, + 0, + id="write-only", + ), + pytest.param( + ["pw", "ev"], + {HAP_REPR_STATUS: HAP_SERVER_STATUS.WRITE_ONLY_CHARACTERISTIC}, + 0, + id="write-notify", + ), + pytest.param( + ["pw", "tw"], + {HAP_REPR_STATUS: HAP_SERVER_STATUS.WRITE_ONLY_CHARACTERISTIC}, + 0, + id="timed-write-only", + ), +] + + +@pytest.mark.parametrize("bridged", [False, True], ids=["standalone", "bridged"]) +@pytest.mark.parametrize(("permissions", "expected", "reads"), READ_PERMISSION_CASES) +def test_get_characteristics_requires_read_permission( + driver: AccessoryDriver, + bridged: bool, + permissions: list[str], + expected: dict[str, Any], + reads: int, +) -> None: + """A read of a characteristic without paired read returns no value.""" + acc, char, getter = _add_bridged_or_standalone(driver, permissions, bridged) + iid = acc.iid_manager.get_iid(char) + + result = driver.get_characteristics([f"{acc.aid}.{iid}"]) + + assert result == { + HAP_REPR_CHARS: [{HAP_REPR_AID: acc.aid, HAP_REPR_IID: iid, **expected}] + } + assert getter.call_count == reads + + +@pytest.mark.parametrize( + ("permissions", "values", "reads"), + [ + (["pr"], {HAP_REPR_VALUE}, 1), + (["pr", "pw", "ev"], {HAP_REPR_VALUE}, 1), + (["pw"], set(), 0), + (["pw", "ev"], set(), 0), + (["pw", "tw"], set(), 0), + ], +) +def test_get_accessories_value_requires_read_permission( + driver: AccessoryDriver, permissions: list[str], values: set[str], reads: int +) -> None: + """The accessory database only includes values the controller may read.""" + acc, char, getter = _add_bridged_or_standalone(driver, permissions, False) + + (acc_rep,) = driver.get_accessories()[HAP_REPR_ACCS] + (char_rep,) = acc_rep[HAP_REPR_SERVICES][-1][HAP_REPR_CHARS] + + assert char_rep[HAP_REPR_IID] == acc.iid_manager.get_iid(char) + assert char_rep.keys() & {HAP_REPR_VALUE} == values + assert getter.call_count == reads + + def test_auto_add_aid_mac(driver: AccessoryDriver) -> None: acc = Accessory(driver, "Test Accessory") driver.add_accessory(acc) From 071f96d22df7ad5ee098fc14ffdc9ef9cc3fab5a Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Sun, 8 Nov 2026 19:27:00 +0100 Subject: [PATCH 02/21] Answer -70406 for event subscriptions without notify permission Any characteristic could be subscribed to, whether or not it has the notify (ev) permission. A controller could then receive the values that another controller wrote to a write-only characteristic. Per the HAP spec, (un)subscribing a characteristic without ev now answers -70406 and leaves the subscriptions unchanged, the same as HAP-NodeJS. A value in the same request entry is not written. Some tests subscribed to Version, which has no ev permission. They now use CurrentDoorState. --- pyhap/accessory_driver.py | 38 +++++++-- tests/test_accessory_driver.py | 146 +++++++++++++++++++++++++++++++++ tests/test_hap_handler.py | 10 +-- tests/test_hap_protocol.py | 4 +- 4 files changed, 183 insertions(+), 15 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index bdbccad0..bfe143cf 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -1021,11 +1021,9 @@ def set_characteristics( :type chars_query: dict """ - # TODO: Add support for chars that do no support notifications. - queries: list[dict[str, Any]] = chars_query[HAP_REPR_CHARS] - self._notify(queries, client_addr) + not_notifiable = self._notify(queries, client_addr) updates_by_accessories_services: dict[ Accessory, dict[Service, dict[Characteristic, Any]] @@ -1045,12 +1043,20 @@ def set_characteristics( assert primary_accessory is not None # noqa: S101 primary_aid = primary_accessory.aid - for query in queries: - if HAP_REPR_VALUE not in query and not expired: - continue + for aid, iid in not_notifiable: + results[aid][iid] = { + HAP_REPR_STATUS: HAP_SERVER_STATUS.NOTIFICATION_NOT_SUPPORTED + } + for query in queries: aid = query[HAP_REPR_AID] iid = query[HAP_REPR_IID] + if (aid, iid) in not_notifiable: + continue + if HAP_REPR_VALUE not in query and not expired: + # Listed with status 0 if the response has other results. + results[aid][iid] = {HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS} + continue value = query.get(HAP_REPR_VALUE) write_response_requested = query.get(HAP_REPR_WRITE_RESPONSE, False) @@ -1184,8 +1190,15 @@ def signal_handler(self, _signal: Any, _frame: Any) -> None: def _notify( self, queries: list[dict[str, Any]], client_addr: tuple[str, int] - ) -> None: - """Notify the driver that the client has subscribed or unsubscribed.""" + ) -> set[tuple[int, int]]: + """Notify the driver that the client has subscribed or unsubscribed. + + Characteristics without the notify permission are left alone; their + ``(aid, iid)`` pairs are returned so the request can report + -70406 for them. + """ + assert self.accessory is not None # noqa: S101 + not_notifiable: set[tuple[int, int]] = set() for query in queries: if HAP_PERMISSION_NOTIFY not in query: continue @@ -1193,7 +1206,16 @@ def _notify( iid = query[HAP_REPR_IID] ev = query[HAP_PERMISSION_NOTIFY] + char = self.accessory.get_characteristic(aid, iid) + if ( + char is not None + and HAP_PERMISSION_NOTIFY not in char.properties[PROP_PERMISSIONS] + ): + not_notifiable.add((aid, iid)) + continue + char_topic = get_topic(aid, iid) action = "Subscribed" if ev else "Unsubscribed" logger.debug("%s client %s to topic %s", action, client_addr, char_topic) self.async_subscribe_client_topic(client_addr, char_topic, ev) + return not_notifiable diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index d4ebbe8f..eddff96b 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -31,6 +31,7 @@ Characteristic, ) from pyhap.const import ( + HAP_PERMISSION_NOTIFY, HAP_PERMISSIONS, HAP_REPR_ACCS, HAP_REPR_AID, @@ -245,6 +246,151 @@ def test_get_accessories_value_requires_read_permission( assert getter.call_count == reads +@pytest.mark.parametrize("bridged", [False, True], ids=["standalone", "bridged"]) +@pytest.mark.parametrize( + "permissions", [["pr", "ev"], ["pr", "pw", "ev"], ["pw", "ev"]] +) +def test_subscribe_with_notify_permission( + driver: AccessoryDriver, bridged: bool, permissions: list[str] +) -> None: + """Characteristics with the notify permission can be (un)subscribed.""" + acc, char, _ = _add_bridged_or_standalone(driver, permissions, bridged) + iid = acc.iid_manager.get_iid(char) + client = ("1.2.3.4", 5) + + subscribe = {HAP_REPR_AID: acc.aid, HAP_REPR_IID: iid, HAP_PERMISSION_NOTIFY: True} + assert driver.set_characteristics({HAP_REPR_CHARS: [subscribe]}, client) is None + assert driver.topics == {f"{acc.aid}.{iid}": {client}} + + unsubscribe = {**subscribe, HAP_PERMISSION_NOTIFY: False} + assert driver.set_characteristics({HAP_REPR_CHARS: [unsubscribe]}, client) is None + assert driver.topics == {} + + +@pytest.mark.parametrize("bridged", [False, True], ids=["standalone", "bridged"]) +@pytest.mark.parametrize("ev", [True, False], ids=["subscribe", "unsubscribe"]) +@pytest.mark.parametrize("permissions", [["pr"], ["pr", "pw"], ["pw"], ["pw", "tw"]]) +def test_subscribe_without_notify_permission( + driver: AccessoryDriver, bridged: bool, ev: bool, permissions: list[str] +) -> None: + """(Un)subscribing a characteristic without notify answers -70406.""" + acc, char, _ = _add_bridged_or_standalone(driver, permissions, bridged) + iid = acc.iid_manager.get_iid(char) + client = ("1.2.3.4", 5) + + result = driver.set_characteristics( + { + HAP_REPR_CHARS: [ + {HAP_REPR_AID: acc.aid, HAP_REPR_IID: iid, HAP_PERMISSION_NOTIFY: ev} + ] + }, + client, + ) + + assert result == { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.NOTIFICATION_NOT_SUPPORTED, + } + ] + } + assert driver.topics == {} + + +def test_refused_subscription_lists_the_others(driver: AccessoryDriver) -> None: + """A 207 for a refused subscription also lists the accepted ones.""" + acc = Accessory(driver, "Test Accessory", aid=STANDALONE_AID) + service = Service(uuid1(), "Test Service") + notifiable = Characteristic( + "Notifiable", + uuid1(), + {PROP_FORMAT: HAP_FORMAT_INT, PROP_PERMISSIONS: ["pr", "ev"]}, + ) + silent = Characteristic( + "Silent", uuid1(), {PROP_FORMAT: HAP_FORMAT_INT, PROP_PERMISSIONS: ["pr"]} + ) + service.add_characteristic(notifiable) + service.add_characteristic(silent) + acc.add_service(service) + driver.add_accessory(acc) + ok_iid = acc.iid_manager.get_iid(notifiable) + refused_iid = acc.iid_manager.get_iid(silent) + client = ("1.2.3.4", 5) + + result = driver.set_characteristics( + { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: ok_iid, + HAP_PERMISSION_NOTIFY: True, + }, + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: refused_iid, + HAP_PERMISSION_NOTIFY: True, + }, + ] + }, + client, + ) + + assert result == { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: refused_iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.NOTIFICATION_NOT_SUPPORTED, + }, + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: ok_iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS, + }, + ] + } + assert driver.topics == {f"{acc.aid}.{ok_iid}": {client}} + + +@pytest.mark.parametrize("ev", [True, False], ids=["subscribe", "unsubscribe"]) +def test_write_with_subscribe_without_notify_permission( + driver: AccessoryDriver, ev: bool +) -> None: + """A write combined with a refused subscription is not applied.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw"], False) + iid = acc.iid_manager.get_iid(char) + setter = MagicMock() + char.setter_callback = setter + + result = driver.set_characteristics( + { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: iid, + HAP_PERMISSION_NOTIFY: ev, + HAP_REPR_VALUE: 5, + } + ] + }, + ("1.2.3.4", 5), + ) + + assert result == { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.NOTIFICATION_NOT_SUPPORTED, + } + ] + } + setter.assert_not_called() + assert driver.topics == {} + + def test_auto_add_aid_mac(driver: AccessoryDriver) -> None: acc = Accessory(driver, "Test Accessory") driver.add_accessory(acc) diff --git a/tests/test_hap_handler.py b/tests/test_hap_handler.py index 98590bea..4ae0378e 100644 --- a/tests/test_hap_handler.py +++ b/tests/test_hap_handler.py @@ -727,7 +727,7 @@ def test_handle_set_handle_set_characteristics_encrypted_pid_missing_prepare( response = hap_handler.HAPResponse() handler.response = response handler.request_body = ( - b'{"pid":123,"characteristics":[{"aid":1,"iid":9,"ev":true}]}' + b'{"pid":123,"characteristics":[{"aid":1,"iid":11,"ev":true}]}' ) handler.handle_set_characteristics() @@ -759,7 +759,7 @@ def test_handle_set_handle_set_characteristics_encrypted_with_prepare( response = hap_handler.HAPResponse() handler.response = response handler.request_body = ( - b'{"pid":123,"characteristics":[{"aid":1,"iid":9,"ev":true}]}' + b'{"pid":123,"characteristics":[{"aid":1,"iid":11,"ev":true}]}' ) handler.handle_set_characteristics() @@ -797,7 +797,7 @@ def test_handle_set_handle_set_characteristics_encrypted_with_multiple_prepare( response = hap_handler.HAPResponse() handler.response = response handler.request_body = ( - b'{"pid":123,"characteristics":[{"aid":1,"iid":9,"ev":true}]}' + b'{"pid":123,"characteristics":[{"aid":1,"iid":11,"ev":true}]}' ) handler.handle_set_characteristics() @@ -851,7 +851,7 @@ def test_handle_set_handle_set_characteristics_encrypted_with_expired_ttl( response = hap_handler.HAPResponse() handler.response = response handler.request_body = ( - b'{"pid":123,"characteristics":[{"aid":1,"iid":9,"ev":true}]}' + b'{"pid":123,"characteristics":[{"aid":1,"iid":11,"ev":true}]}' ) handler.handle_set_characteristics() @@ -883,7 +883,7 @@ def test_handle_set_handle_set_characteristics_encrypted_with_wrong_pid( response = hap_handler.HAPResponse() handler.response = response handler.request_body = ( - b'{"pid":456,"characteristics":[{"aid":1,"iid":9,"ev":true}]}' + b'{"pid":456,"characteristics":[{"aid":1,"iid":11,"ev":true}]}' ) handler.handle_set_characteristics() diff --git a/tests/test_hap_protocol.py b/tests/test_hap_protocol.py index c5b0ad37..749e5b00 100644 --- a/tests/test_hap_protocol.py +++ b/tests/test_hap_protocol.py @@ -433,10 +433,10 @@ def test_set_characteristics_with_crypto(driver: AccessoryDriver) -> None: hap_proto.data_received( b"PUT /characteristics HTTP/1.1\r\n" b"Host: HASS12\\032AD1C22._hap._tcp.local\r\n" - b"Content-Length: 49\r\n" + b"Content-Length: 50\r\n" b"Content-Type: application/hap+json\r\n" b"\r\n" - b'{"characteristics":[{"aid":1,"iid":9,"ev":true}]}' + b'{"characteristics":[{"aid":1,"iid":11,"ev":true}]}' ) hap_proto.close() From 3fd5a1b451b085c44180a7b698ce6e5d583dc007 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Sun, 8 Nov 2026 19:45:10 +0100 Subject: [PATCH 03/21] Return -70404 when writing a characteristic without paired write A value written to a characteristic without the pw permission still reached its setter. Answer it with -70404 like HAP-NodeJS instead, without calling the characteristic, service or accessory setters. A subscription in the same entry and the other characteristics of the request are still handled. --- pyhap/accessory_driver.py | 10 +++ tests/test_accessory_driver.py | 132 ++++++++++++++++++++++++++++++++- tests/test_hap_handler.py | 2 +- 3 files changed, 142 insertions(+), 2 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index bfe143cf..c9f97599 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -40,6 +40,7 @@ from pyhap.const import ( HAP_PERMISSION_NOTIFY, HAP_PERMISSION_READ, + HAP_PERMISSION_WRITE, HAP_PROTOCOL_SHORT_VERSION, HAP_REPR_ACCS, HAP_REPR_AID, @@ -1075,6 +1076,15 @@ def set_characteristics( if char is None: continue + if ( + value is not None + and HAP_PERMISSION_WRITE not in char.properties[PROP_PERMISSIONS] + ): + results[aid][iid] = { + HAP_REPR_STATUS: HAP_SERVER_STATUS.READ_ONLY_CHARACTERISTIC + } + continue + set_result = HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST set_result_value = None diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index eddff96b..92c582ac 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -32,6 +32,7 @@ ) from pyhap.const import ( HAP_PERMISSION_NOTIFY, + HAP_PERMISSION_WRITE, HAP_PERMISSIONS, HAP_REPR_ACCS, HAP_REPR_AID, @@ -52,7 +53,7 @@ CHAR_PROPS = { PROP_FORMAT: HAP_FORMAT_INT, - PROP_PERMISSIONS: HAP_PERMISSION_READ, + PROP_PERMISSIONS: [HAP_PERMISSION_READ, HAP_PERMISSION_WRITE], } @@ -391,6 +392,135 @@ def test_write_with_subscribe_without_notify_permission( assert driver.topics == {} +def _write_value( + driver: AccessoryDriver, acc: Accessory, char: Characteristic, **query: Any +) -> tuple[dict[str, Any] | None, MagicMock, MagicMock]: + """Write 5 to ``char`` and return the result and both setter mocks.""" + setter = MagicMock() + char.setter_callback = setter + service_setter = MagicMock() + assert char.service is not None + char.service.setter_callback = service_setter + iid = acc.iid_manager.get_iid(char) + result = driver.set_characteristics( + { + HAP_REPR_CHARS: [ + {HAP_REPR_AID: acc.aid, HAP_REPR_IID: iid, HAP_REPR_VALUE: 5, **query} + ] + }, + ("1.2.3.4", 5), + ) + return result, setter, service_setter + + +@pytest.mark.parametrize("bridged", [False, True], ids=["standalone", "bridged"]) +@pytest.mark.parametrize("permissions", [["pr"], ["pr", "ev"]]) +def test_write_without_write_permission( + driver: AccessoryDriver, bridged: bool, permissions: list[str] +) -> None: + """A write to a characteristic without paired write is refused.""" + acc, char, _ = _add_bridged_or_standalone(driver, permissions, bridged) + + result, setter, service_setter = _write_value(driver, acc, char) + + assert result == { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: acc.iid_manager.get_iid(char), + HAP_REPR_STATUS: HAP_SERVER_STATUS.READ_ONLY_CHARACTERISTIC, + } + ] + } + setter.assert_not_called() + service_setter.assert_not_called() + assert char.value == 0 + + +@pytest.mark.parametrize("bridged", [False, True], ids=["standalone", "bridged"]) +@pytest.mark.parametrize("permissions", [["pw"], ["pr", "pw"], ["pr", "pw", "ev"]]) +def test_write_with_write_permission( + driver: AccessoryDriver, bridged: bool, permissions: list[str] +) -> None: + """A write to a characteristic with paired write is applied.""" + acc, char, _ = _add_bridged_or_standalone(driver, permissions, bridged) + + result, setter, service_setter = _write_value(driver, acc, char) + + assert result is None + setter.assert_called_once_with(5) + service_setter.assert_called_once_with({char.display_name: 5}) + assert char.value == 5 + + +def test_write_with_subscribe_without_write_permission( + driver: AccessoryDriver, +) -> None: + """A subscription is applied even when the write in the same entry is not.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "ev"], False) + client = ("1.2.3.4", 5) + iid = acc.iid_manager.get_iid(char) + + result, setter, _ = _write_value(driver, acc, char, **{HAP_PERMISSION_NOTIFY: True}) + + assert result == { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.READ_ONLY_CHARACTERISTIC, + } + ] + } + setter.assert_not_called() + assert driver.topics == {f"{acc.aid}.{iid}": {client}} + + +def test_refused_write_does_not_block_others(driver: AccessoryDriver) -> None: + """A refused write leaves the other characteristics of the request alone.""" + acc = Accessory(driver, "Test Accessory", aid=STANDALONE_AID) + service = Service(uuid1(), "Test Service") + read_only = Characteristic( + "ReadOnly", uuid1(), {PROP_FORMAT: HAP_FORMAT_INT, PROP_PERMISSIONS: ["pr"]} + ) + writable = Characteristic("Writable", uuid1(), CHAR_PROPS) + service.add_characteristic(read_only, writable) + acc.add_service(service) + driver.add_accessory(acc) + setter = MagicMock() + writable.setter_callback = setter + read_only_iid = acc.iid_manager.get_iid(read_only) + writable_iid = acc.iid_manager.get_iid(writable) + + result = driver.set_characteristics( + { + HAP_REPR_CHARS: [ + {HAP_REPR_AID: acc.aid, HAP_REPR_IID: read_only_iid, HAP_REPR_VALUE: 5}, + {HAP_REPR_AID: acc.aid, HAP_REPR_IID: writable_iid, HAP_REPR_VALUE: 5}, + ] + }, + ("1.2.3.4", 5), + ) + + assert result == { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: read_only_iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.READ_ONLY_CHARACTERISTIC, + }, + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: writable_iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS, + }, + ] + } + setter.assert_called_once_with(5) + assert read_only.value == 0 + assert writable.value == 5 + + def test_auto_add_aid_mac(driver: AccessoryDriver) -> None: acc = Accessory(driver, "Test Accessory") driver.add_accessory(acc) diff --git a/tests/test_hap_handler.py b/tests/test_hap_handler.py index 4ae0378e..9dc91435 100644 --- a/tests/test_hap_handler.py +++ b/tests/test_hap_handler.py @@ -930,7 +930,7 @@ def _mock_failure(*_: Any) -> NoReturn: response = hap_handler.HAPResponse() handler.response = response - handler.request_body = b'{"characteristics":[{"aid":1,"iid":11,"value":1}]}' + handler.request_body = b'{"characteristics":[{"aid":1,"iid":12,"value":1}]}' handler.handle_set_characteristics() assert response.status_code == 207 From 93d17d357687823b77fea8ea94a8c1a5db77be97 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Sun, 8 Nov 2026 20:07:16 +0100 Subject: [PATCH 04/21] Ignore timed writes with an unknown or expired pid A write carrying a pid that was never prepared on this connection, was already used or has outlived its TTL was still applied, and its event subscriptions were changed. Only characteristics without a value got -70410. The HAP specification says such a request must be ignored and answered with -70410. Do that for every characteristic in the request, as HAP-NodeJS does. The TTL is now measured with the monotonic clock, so a wall clock jump, e.g. from NTP right after boot, can't expire a timed write. --- pyhap/accessory_driver.py | 31 +++++-- tests/test_accessory_driver.py | 2 +- tests/test_timed_write.py | 164 +++++++++++++++++++++++++++++++++ 3 files changed, 188 insertions(+), 9 deletions(-) create mode 100644 tests/test_timed_write.py diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index bd05a8c2..ca4a61f7 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -1043,6 +1043,22 @@ def set_characteristics( """ queries: list[dict[str, Any]] = chars_query[HAP_REPR_CHARS] + if HAP_REPR_PID in chars_query 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[ @@ -1053,12 +1069,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 @@ -1073,7 +1083,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 @@ -1170,6 +1180,11 @@ 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.""" + expires_at = self.prepared_writes.get(client_addr, {}).pop(pid, None) + return expires_at is not None and time.monotonic() <= expires_at + def prepare( self, prepare_query: dict[str, Any], client_addr: tuple[str, int] ) -> dict[str, int]: @@ -1189,7 +1204,7 @@ def prepare( 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 diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index 475a3c7f..a2975924 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -1817,7 +1817,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..a0f5c969 --- /dev/null +++ b/tests/test_timed_write.py @@ -0,0 +1,164 @@ +"""Tests for timed writes (prepare, then write with the pid).""" + +from collections.abc import Iterator +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 + + +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), (7, 2.6)], + ids=["unknown_pid", "expired_pid"], +) +def test_unprepared_write_is_ignored( + driver: AccessoryDriver, + door: WritableChar, + write_pid: int, + 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: 7}, 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_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() From e09ecbdd58dd6119b98456d0431d774d51fdd272 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Sun, 8 Nov 2026 19:33:23 +0100 Subject: [PATCH 05/21] Split building the characteristics write response into helpers No behaviour change. The next commit fills in the response after awaiting async setters, which needs the result building in the driver and the HTTP response filling in the handler to work on their own. --- pyhap/accessory_driver.py | 35 +++++++++++++++++++++-------------- pyhap/hap_handler.py | 22 +++++++++++++++------- 2 files changed, 36 insertions(+), 21 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index 5ab915e2..7794081f 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -156,6 +156,26 @@ def _wrap_service_setter(service: Any, chars: Any, client_addr: Any) -> Any: return HAP_SERVER_STATUS.SUCCESS +def _set_characteristics_response( + results: dict[int, dict[int, dict[str, Any]]], +) -> dict[str, Any] | None: + """Build the write response, or None if every write succeeded without a value.""" + characteristics = [] + nonempty_results_exist = False + for aid, iid_results in results.items(): + for iid, result in iid_results.items(): + result[HAP_REPR_AID] = aid + result[HAP_REPR_IID] = iid + characteristics.append(result) + if ( + result[HAP_REPR_STATUS] != HAP_SERVER_STATUS.SUCCESS + or HAP_REPR_VALUE in result + ): + nonempty_results_exist = True + + return {HAP_REPR_CHARS: characteristics} if nonempty_results_exist else None + + class AccessoryMDNSServiceInfo(ServiceInfo): """A mDNS service info representation of an accessory.""" @@ -1194,20 +1214,7 @@ def set_characteristics( for char in chars: aid_results[char_to_iid[char]][HAP_REPR_STATUS] = set_result - characteristics = [] - nonempty_results_exist = False - for aid, iid_results in results.items(): - for iid, result in iid_results.items(): - result[HAP_REPR_AID] = aid - result[HAP_REPR_IID] = iid - characteristics.append(result) - if ( - result[HAP_REPR_STATUS] != HAP_SERVER_STATUS.SUCCESS - or HAP_REPR_VALUE in result - ): - nonempty_results_exist = True - - return {HAP_REPR_CHARS: characteristics} if nonempty_results_exist else None + return _set_characteristics_response(results) 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.""" diff --git a/pyhap/hap_handler.py b/pyhap/hap_handler.py index 83a47380..a30a388b 100644 --- a/pyhap/hap_handler.py +++ b/pyhap/hap_handler.py @@ -711,16 +711,24 @@ def handle_set_characteristics(self) -> None: "%s: Set characteristics content: %s", self.client_address, requested_chars ) - response = self.accessory_handler.set_characteristics( + result = self.accessory_handler.set_characteristics( requested_chars, cast("tuple[str, int]", self.client_address) ) - if response is None: - self.send_response(HTTPStatus.NO_CONTENT) - return + assert self.response is not None # noqa: S101 + self._fill_set_characteristics_response(self.response, result) - self.send_response(HTTPStatus.MULTI_STATUS) - self.send_header("Content-Type", self.JSON_RESPONSE_TYPE) - self.end_response(to_hap_json(response)) + def _fill_set_characteristics_response( + self, response: HAPResponse, result: dict[str, Any] | None + ) -> None: + """Fill in the response to a characteristics write.""" + if result is None: + response.status_code = HTTPStatus.NO_CONTENT.value + response.reason = HTTPStatus.NO_CONTENT.phrase + return + response.status_code = HTTPStatus.MULTI_STATUS.value + response.reason = HTTPStatus.MULTI_STATUS.phrase + response.headers.append(("Content-Type", self.JSON_RESPONSE_TYPE)) + response.body = to_hap_json(result) def handle_prepare(self) -> None: """Handles a client request to prepare to write.""" From 1ef0721abc04d43ec4367203fbd97151e439394f Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Sun, 8 Nov 2026 19:28:30 +0100 Subject: [PATCH 06/21] Only send write responses for characteristics with wr permission A write with "r": true got the setter's return value back for any characteristic. The HAP spec ties write responses to the write response (wr) permission, and HAP-NodeJS ignores the request for other characteristics. Do the same. When a write response is sent, it becomes the characteristic's value instead of the written one, which HAP-NodeJS does as well. The sent value is the stored one, after the usual format checks. A response the characteristic can't hold is still sent and the written value is kept. The write response tests are now parametrized and also cover a wr characteristic without paired read: neither the written value nor the response can be read back. --- pyhap/accessory_driver.py | 33 ++++++- pyhap/characteristic.py | 4 +- tests/test_accessory_driver.py | 169 +++++++++++++++++---------------- 3 files changed, 122 insertions(+), 84 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index c9f97599..bd05a8c2 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -41,6 +41,7 @@ HAP_PERMISSION_NOTIFY, HAP_PERMISSION_READ, HAP_PERMISSION_WRITE, + HAP_PERMISSION_WRITE_RESPONSE, HAP_PROTOCOL_SHORT_VERSION, HAP_REPR_ACCS, HAP_REPR_AID, @@ -87,6 +88,24 @@ MAX_PREPARED_WRITE_TTL_SECONDS = 30.0 +def _store_write_response(char: Characteristic, response: Any) -> Any: + """Keep the write response as the value and return what to send. + + The written value of a write response characteristic, such as an SDP + offer, is a request. What the accessory answered is its state. + """ + try: + char.value = char.to_valid_value(response) + except ValueError: + logger.debug( + "%s: keeping the written value, the write response is invalid: %s", + char.display_name, + response, + ) + return response + return char.value + + def _wrap_char_setter(char: Any, value: Any, client_addr: Any) -> tuple[int, Any]: """Process an characteristic setter callback trapping and logging all exceptions.""" try: @@ -1059,7 +1078,6 @@ def set_characteristics( results[aid][iid] = {HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS} continue value = query.get(HAP_REPR_VALUE) - write_response_requested = query.get(HAP_REPR_WRITE_RESPONSE, False) if aid == primary_aid: acc = primary_accessory @@ -1093,8 +1111,17 @@ def set_characteristics( char, value, client_addr ) - if set_result_value is not None and write_response_requested: - result = {HAP_REPR_STATUS: set_result, HAP_REPR_VALUE: set_result_value} + # Only characteristics with the write response permission answer + # a write with a value, and only when the controller asks for it. + if ( + set_result_value is not None + and query.get(HAP_REPR_WRITE_RESPONSE, False) + and HAP_PERMISSION_WRITE_RESPONSE in char.properties[PROP_PERMISSIONS] + ): + result = { + HAP_REPR_STATUS: set_result, + HAP_REPR_VALUE: _store_write_response(char, set_result_value), + } else: result = {HAP_REPR_STATUS: set_result} diff --git a/pyhap/characteristic.py b/pyhap/characteristic.py index bcbdd3e2..a0b09a69 100644 --- a/pyhap/characteristic.py +++ b/pyhap/characteristic.py @@ -201,7 +201,9 @@ def __init__( self._value = self._get_default_value() self.getter_callback: Callable[[], Any] | None = None # Setters may take just the value or (value, sender_client_addr); see - # ``_setter_wants_addr``. The return value is used for write responses. + # ``_setter_wants_addr``. The return value is sent back as the write + # response when the characteristic has the "wr" permission and the + # controller asked for one. self.setter_callback: Callable[..., Any] | None = None # Cache of whether ``setter_callback`` opts into the client address, # recomputed only when the setter object itself changes. diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index 92c582ac..475a3c7f 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -609,109 +609,118 @@ def test_listen_address(kwargs: dict[str, Any], expected: Any) -> None: assert server.call_args.args[0] == (expected, 51234) -def test_write_response_returned_when_not_requested(driver: AccessoryDriver) -> None: - bridge = Bridge(driver, "mybridge") - acc = Accessory(driver, "TestAcc", aid=2) - service = Service(uuid1(), "NFCAccess") - char_nfc_access_control_point = Characteristic( - "NFCAccessControlPoint", uuid1(), CHAR_PROPS - ) - service.add_characteristic(char_nfc_access_control_point) - - mock_callback = MagicMock() - service.setter_callback = mock_callback - - def setter_with_write_response(value: int = 0) -> int: - return 1 - - char_nfc_access_control_point.setter_callback = setter_with_write_response - acc.add_service(service) - - char_nfc_access_control_point_iid = char_nfc_access_control_point.to_HAP()[ - HAP_REPR_IID - ] - - bridge.add_accessory(acc) - driver.add_accessory(bridge) - - response = driver.set_characteristics( +def _write( + driver: AccessoryDriver, acc: Accessory, char: Characteristic, **query: Any +) -> dict[str, Any] | None: + """Write 0 to ``char`` from a mock client with extra query fields.""" + return driver.set_characteristics( { HAP_REPR_CHARS: [ { HAP_REPR_AID: acc.aid, - HAP_REPR_IID: char_nfc_access_control_point_iid, + HAP_REPR_IID: acc.iid_manager.get_iid(char), HAP_REPR_VALUE: 0, - HAP_REPR_WRITE_RESPONSE: False, + **query, } ] }, - "mock_addr", # ty: ignore[invalid-argument-type] + ("1.2.3.4", 5), ) - assert response is None - response = driver.set_characteristics( - { - HAP_REPR_CHARS: [ - { - HAP_REPR_AID: acc.aid, - HAP_REPR_IID: char_nfc_access_control_point_iid, - HAP_REPR_VALUE: 0, - } - ] - }, - "mock_addr", # ty: ignore[invalid-argument-type] - ) - assert response is None +@pytest.mark.parametrize("bridged", [False, True], ids=["standalone", "bridged"]) +@pytest.mark.parametrize("permissions", [["pr", "pw", "wr"], ["pw", "wr"]]) +@pytest.mark.parametrize(("returned", "sent"), [(1, 1), (1.7, 1)]) +def test_write_response_returned_when_requested( + driver: AccessoryDriver, + bridged: bool, + permissions: list[str], + returned: Any, + sent: int, +) -> None: + """A write response carries the setter's value as the char stores it.""" + acc, char, _ = _add_bridged_or_standalone(driver, permissions, bridged) + service_setter = MagicMock() + assert char.service is not None + char.service.setter_callback = service_setter + char.setter_callback = MagicMock(return_value=returned) -def test_write_response_returned_when_requested(driver: AccessoryDriver) -> None: - bridge = Bridge(driver, "mybridge") - acc = Accessory(driver, "TestAcc", aid=2) - service = Service(uuid1(), "NFCAccess") - char_nfc_access_control_point = Characteristic( - "NFCAccessControlPoint", uuid1(), CHAR_PROPS - ) - service.add_characteristic(char_nfc_access_control_point) + response = _write(driver, acc, char, **{HAP_REPR_WRITE_RESPONSE: True}) - mock_callback = MagicMock() - service.setter_callback = mock_callback + assert response == { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: acc.iid_manager.get_iid(char), + HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS, + HAP_REPR_VALUE: sent, + } + ] + } + char.setter_callback.assert_called_once_with(0) + service_setter.assert_called_once_with({"Value": 0}) + # Like HAP-NodeJS, the response is the value from now on. + assert char.value == sent - def setter_with_write_response(value: int = 0) -> int: - return 1 - char_nfc_access_control_point.setter_callback = setter_with_write_response - acc.add_service(service) +@pytest.mark.parametrize( + ("permissions", "query"), + [ + pytest.param(["pr", "pw", "wr"], {}, id="not-requested"), + pytest.param( + ["pr", "pw", "wr"], {HAP_REPR_WRITE_RESPONSE: False}, id="declined" + ), + pytest.param( + ["pr", "pw"], {HAP_REPR_WRITE_RESPONSE: True}, id="no-wr-permission" + ), + pytest.param(["pw"], {HAP_REPR_WRITE_RESPONSE: True}, id="write-only"), + ], +) +def test_write_response_not_returned( + driver: AccessoryDriver, permissions: list[str], query: dict[str, Any] +) -> None: + """Without a request or the wr permission the write has no response body.""" + acc, char, _ = _add_bridged_or_standalone(driver, permissions, False) + char.setter_callback = MagicMock(return_value=1) - char_nfc_access_control_point_iid = char_nfc_access_control_point.to_HAP()[ - HAP_REPR_IID - ] + assert _write(driver, acc, char, **query) is None + char.setter_callback.assert_called_once_with(0) + assert char.value == 0 - bridge.add_accessory(acc) - driver.add_accessory(bridge) - response = driver.set_characteristics( - { - HAP_REPR_CHARS: [ - { - HAP_REPR_AID: acc.aid, - HAP_REPR_IID: char_nfc_access_control_point_iid, - HAP_REPR_VALUE: 0, - HAP_REPR_WRITE_RESPONSE: True, - } - ] - }, - "mock_addr", # ty: ignore[invalid-argument-type] - ) - assert response == { +def test_invalid_write_response_keeps_written_value(driver: AccessoryDriver) -> None: + """A response the char can't hold is still sent, the written value stays.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "wr"], False) + char.setter_callback = MagicMock(return_value="not a number") + + response = _write(driver, acc, char, **{HAP_REPR_WRITE_RESPONSE: True}) + + assert response is not None + assert response[HAP_REPR_CHARS][0][HAP_REPR_VALUE] == "not a number" + assert char.value == 0 + + +def test_write_response_value_of_write_only_char_not_readable( + driver: AccessoryDriver, +) -> None: + """Neither the written nor the response value of a wr char without pr leaks.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pw", "wr"], False) + char.setter_callback = MagicMock(return_value=1) + iid = acc.iid_manager.get_iid(char) + + _write(driver, acc, char, **{HAP_REPR_WRITE_RESPONSE: True}) + + assert driver.get_characteristics([f"{acc.aid}.{iid}"]) == { HAP_REPR_CHARS: [ { HAP_REPR_AID: acc.aid, - HAP_REPR_IID: char_nfc_access_control_point_iid, - HAP_REPR_STATUS: 0, - HAP_REPR_VALUE: 1, - }, + HAP_REPR_IID: iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.WRITE_ONLY_CHARACTERISTIC, + } ] } + (acc_rep,) = driver.get_accessories()[HAP_REPR_ACCS] + assert HAP_REPR_VALUE not in acc_rep[HAP_REPR_SERVICES][-1][HAP_REPR_CHARS][0] def test_service_callbacks(driver: AccessoryDriver) -> None: From 9ef52f7aac961ab0f886a35e02380b2b3900c09a Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Sun, 8 Nov 2026 20:08:03 +0100 Subject: [PATCH 07/21] Check the timed write permission on writes The HAP specification requires a plain write to a characteristic with the timed write ("tw") permission to be refused with -70410. HAP-NodeJS does this. A Home hub has been seen writing HomeKit Camera Active without a timed write, and the HomeKit Secure Video guide gives Streaming Enabled and Motion Enabled tw as well. Other controllers may not support timed writes at all. So the check is opt-in: with enforce_timed_writes=True the driver refuses such a write, otherwise it applies it and only logs at debug level. --- pyhap/accessory_driver.py | 31 +++++++++++++++- pyhap/const.py | 1 + tests/test_timed_write.py | 78 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 1 deletion(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index ca4a61f7..5ab915e2 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -40,6 +40,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, @@ -249,6 +250,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. @@ -303,6 +305,15 @@ 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. When False, such a write is + applied and only logged at debug level: a Home hub has been seen + writing HomeKit Camera Active without a timed write, and the HomeKit + Secure Video guide gives Streaming Enabled and Motion Enabled tw too, + so enforcing it would break HKSV with such hubs. + :type enforce_timed_writes: bool """ if loop is None: if sys.platform == "win32": @@ -335,6 +346,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 @@ -1043,7 +1055,8 @@ def set_characteristics( """ queries: list[dict[str, Any]] = chars_query[HAP_REPR_CHARS] - if HAP_REPR_PID in chars_query and not self._take_prepared_write( + 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 @@ -1113,6 +1126,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, + ) + set_result = HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST set_result_value = None 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_timed_write.py b/tests/test_timed_write.py index a0f5c969..cac75f8b 100644 --- a/tests/test_timed_write.py +++ b/tests/test_timed_write.py @@ -1,6 +1,7 @@ """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 @@ -162,3 +163,80 @@ def test_prepared_pid_belongs_to_its_connection( HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST ) door.setter.assert_not_called() + + +def _rejection(char: WritableChar, status: int | None) -> dict[str, Any] | None: + """Return the expected write response, None for a plain success.""" + if status is None: + return None + return { + HAP_REPR_CHARS: [ + {HAP_REPR_AID: char.aid, HAP_REPR_IID: char.iid, HAP_REPR_STATUS: status} + ] + } + + +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) + + +REJECTED = HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST + + +@pytest.mark.parametrize( + ("enforce", "name", "request_extra", "status", "calls", "logged"), + [ + (True, "HomeKitCameraActive", {}, REJECTED, 0, 0), + (True, "HomeKitCameraActive", {HAP_REPR_PID: 7}, None, 1, 0), + (True, "TargetDoorState", {}, None, 1, 0), + (False, "HomeKitCameraActive", {}, None, 1, 1), + (False, "HomeKitCameraActive", {HAP_REPR_PID: 7}, None, 1, 0), + (False, "TargetDoorState", {}, None, 1, 0), + ], + ids=[ + "enforced_plain_write", + "enforced_timed_write", + "enforced_plain_write_without_tw", + "lenient_plain_write", + "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], + status: int | None, + 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: 0} + ], + **request_extra, + } + + response = driver.set_characteristics(query, CLIENT) + + assert response == _rejection(char, status) + 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 From 3082bbac7a7aa0f27ccef1191b750de0776c23bd Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Sun, 8 Nov 2026 20:05:41 +0100 Subject: [PATCH 08/21] Await async characteristic setters before answering the write A setter that is a coroutine function was called but never awaited. With "r": true the coroutine was passed to the JSON encoder, which raised TypeError, so the whole write failed with a 500. Writes from a controller now wait for coroutine setters before they are answered, through the existing delayed response. Accessory and service setters still run first. A setter still running after the response timeout is cancelled and its characteristic answers -70408; the other characteristics keep their results. A setter that raises answers -70402, and a closed connection cancels the setters. The return value becomes the write response and the stored value under the same rules as for sync setters. The public set_characteristics() keeps its signature. Called directly, it schedules coroutine setters and answers at once. Only coroutines are awaited; a sync setter that returns a task or future works as before. --- pyhap/accessory_driver.py | 161 +++++++++++-- pyhap/characteristic.py | 6 +- pyhap/hap_handler.py | 21 +- tests/test_accessory_driver.py | 419 ++++++++++++++++++++++++++++++++- tests/test_hap_handler.py | 88 ++++++- tests/test_hap_protocol.py | 95 ++++++++ 6 files changed, 761 insertions(+), 29 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index 7794081f..7a21457f 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -20,8 +20,10 @@ import asyncio import base64 from collections import defaultdict +from collections.abc import Awaitable, Coroutine from concurrent.futures import ThreadPoolExecutor import hashlib +import inspect import logging import os import re @@ -122,10 +124,24 @@ def _wrap_char_setter(char: Any, value: Any, client_addr: Any) -> tuple[int, Any return HAP_SERVER_STATUS.SUCCESS, response +def _close_unsupported_coroutine(result: Any, name: str) -> None: + """Close a coroutine an accessory or service setter returned. + + Only characteristic setters may be coroutine functions. + """ + if inspect.iscoroutine(result): + result.close() + logger.warning( + "%s: only characteristic setters may be coroutine functions", name + ) + + def _wrap_acc_setter(acc: Any, updates_by_service: Any, client_addr: Any) -> Any: """Process an accessory setter callback trapping and logging all exceptions.""" try: - acc.setter_callback(updates_by_service) + _close_unsupported_coroutine( + acc.setter_callback(updates_by_service), acc.display_name + ) except Exception: logger.exception( "%s: Error while setting characteristics to %s for the %s accessory", @@ -144,7 +160,9 @@ def _wrap_service_setter(service: Any, chars: Any, client_addr: Any) -> Any: # consumers of the data. service_chars = {char.display_name: value for char, value in chars.items()} try: - service.setter_callback(service_chars) + _close_unsupported_coroutine( + service.setter_callback(service_chars), service.display_name + ) except Exception: logger.exception( "%s: Error while setting characteristics to %s for the %s service", @@ -156,6 +174,11 @@ def _wrap_service_setter(service: Any, chars: Any, client_addr: Any) -> Any: return HAP_SERVER_STATUS.SUCCESS +# A characteristic write whose setter is a coroutine: the result entry to +# complete, the coroutine, whether a write response was asked for and the char. +_PendingWrite = tuple[dict[str, Any], Coroutine[Any, Any, Any], bool, Characteristic] + + def _set_characteristics_response( results: dict[int, dict[int, dict[str, Any]]], ) -> dict[str, Any] | None: @@ -1057,6 +1080,15 @@ def set_characteristics( ) -> dict[str, Any] | None: """Called from ``HAPServerHandler`` when iOS configures the characteristics. + Returns ``None`` when every write succeeded without a write response, + otherwise the per-characteristic results. + + A characteristic setter may be a coroutine function. Writes from a + controller wait for it before they are answered. Called directly, this + method only schedules the coroutine and does not wait for it. + Accessory and service setters must not be coroutine functions; a + coroutine they return is closed with a warning. + :param chars_query: A configuration query. For example: .. code-block:: python @@ -1073,6 +1105,52 @@ def set_characteristics( :type chars_query: dict """ + results, pending = self._apply_characteristic_writes(chars_query, client_addr) + for _, coro, _, _ in pending: + self.add_job(coro) + return _set_characteristics_response(results) + + def _set_characteristics_for_controller( + self, + chars_query: dict[str, Any], + client_addr: tuple[str, int], + timeout: float, + ) -> dict[str, Any] | None | Awaitable[dict[str, Any] | None]: + """Apply a controller's write, waiting for coroutine setters. + + Returns an awaitable of the response if a characteristic setter is a + coroutine function. Accessory and service setters run before those + coroutines start. A setter still running after ``timeout`` seconds is + cancelled and its characteristic answers -70408; cancelling the + awaitable, e.g. because the connection closed, cancels the setters. + """ + results, pending = self._apply_characteristic_writes(chars_query, client_addr) + if pending: + return self._async_finish_set_characteristics( + results, pending, client_addr, timeout + ) + return _set_characteristics_response(results) + + def _apply_characteristic_writes( + self, chars_query: dict[str, Any], client_addr: tuple[str, int] + ) -> tuple[dict[int, dict[int, dict[str, Any]]], list[_PendingWrite]]: + """Run the setters of a write and collect the per-characteristic results.""" + pending: list[_PendingWrite] = [] + try: + return self._run_characteristic_writes(chars_query, client_addr, pending) + except BaseException: + # A malformed later entry must not leave a setter coroutine + # un-awaited. + for _, coro, _, _ in pending: + coro.close() + raise + + def _run_characteristic_writes( + self, + chars_query: dict[str, Any], + client_addr: tuple[str, int], + pending: list[_PendingWrite], + ) -> tuple[dict[int, dict[int, dict[str, Any]]], list[_PendingWrite]]: queries: list[dict[str, Any]] = chars_query[HAP_REPR_CHARS] timed_write = HAP_REPR_PID in chars_query @@ -1081,16 +1159,12 @@ def set_characteristics( ): # 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 - ] - } + ignored: dict[int, dict[int, dict[str, Any]]] = defaultdict(dict) + for query in queries: + ignored[query[HAP_REPR_AID]][query[HAP_REPR_IID]] = { + HAP_REPR_STATUS: HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST + } + return ignored, [] not_notifiable = self._notify(queries, client_addr) @@ -1172,17 +1246,15 @@ def set_characteristics( # Only characteristics with the write response permission answer # a write with a value, and only when the controller asks for it. - if ( - set_result_value is not None - and query.get(HAP_REPR_WRITE_RESPONSE, False) + write_response = bool( + query.get(HAP_REPR_WRITE_RESPONSE, False) and HAP_PERMISSION_WRITE_RESPONSE in char.properties[PROP_PERMISSIONS] - ): - result = { - HAP_REPR_STATUS: set_result, - HAP_REPR_VALUE: _store_write_response(char, set_result_value), - } - else: - result = {HAP_REPR_STATUS: set_result} + ) + result: dict[str, Any] = {HAP_REPR_STATUS: set_result} + if inspect.iscoroutine(set_result_value): + pending.append((result, set_result_value, write_response, char)) + elif set_result_value is not None and write_response: + result[HAP_REPR_VALUE] = _store_write_response(char, set_result_value) results[aid][iid] = result char_to_iid[char] = iid @@ -1214,6 +1286,51 @@ def set_characteristics( for char in chars: aid_results[char_to_iid[char]][HAP_REPR_STATUS] = set_result + return results, pending + + async def _async_finish_set_characteristics( + self, + results: dict[int, dict[int, dict[str, Any]]], + pending: list[_PendingWrite], + client_addr: tuple[str, int], + timeout: float, + ) -> dict[str, Any] | None: + """Wait for the coroutine setters, then build the response.""" + tasks = { + asyncio.ensure_future(coro): (result, write_response, char) + for result, coro, write_response, char in pending + } + try: + done, not_done = await asyncio.wait(tasks, timeout=timeout) + except asyncio.CancelledError: + for task in tasks: + task.cancel() + raise + for task in not_done: + task.cancel() + result, _, char = tasks[task] + logger.warning( + "%s: Timed out setting characteristic %s", + client_addr, + char.display_name, + ) + result[HAP_REPR_STATUS] = HAP_SERVER_STATUS.OPERATION_TIMED_OUT + for task in done: + result, write_response, char = tasks[task] + try: + outcome = task.result() + except (Exception, asyncio.CancelledError): + logger.exception( + "%s: Error while setting characteristic %s", + client_addr, + char.display_name, + ) + result[HAP_REPR_STATUS] = ( + HAP_SERVER_STATUS.SERVICE_COMMUNICATION_FAILURE + ) + continue + if outcome is not None and write_response: + result[HAP_REPR_VALUE] = _store_write_response(char, outcome) return _set_characteristics_response(results) def _take_prepared_write(self, pid: Any, client_addr: tuple[str, int]) -> bool: diff --git a/pyhap/characteristic.py b/pyhap/characteristic.py index a0b09a69..4472638b 100644 --- a/pyhap/characteristic.py +++ b/pyhap/characteristic.py @@ -203,7 +203,8 @@ def __init__( # Setters may take just the value or (value, sender_client_addr); see # ``_setter_wants_addr``. The return value is sent back as the write # response when the characteristic has the "wr" permission and the - # controller asked for one. + # controller asked for one. A setter may also be a coroutine function; + # a controller's write is answered once it is done. self.setter_callback: Callable[..., Any] | None = None # Cache of whether ``setter_callback`` opts into the client address, # recomputed only when the setter object itself changes. @@ -398,7 +399,8 @@ def client_update_value( ) -> Any: """Called from broker for value change in Home app. - Change self.value to value and call callback. + Change self.value to value and call callback. Returns what the callback + returned, which the caller has to await if it is awaitable. """ original_value = value if not self._always_null or original_value is not None: diff --git a/pyhap/hap_handler.py b/pyhap/hap_handler.py index a30a388b..1dce8a06 100644 --- a/pyhap/hap_handler.py +++ b/pyhap/hap_handler.py @@ -6,6 +6,7 @@ import asyncio from collections.abc import Awaitable from http import HTTPStatus +import inspect import logging import sys from typing import TYPE_CHECKING, Any, cast @@ -711,10 +712,19 @@ def handle_set_characteristics(self) -> None: "%s: Set characteristics content: %s", self.client_address, requested_chars ) - result = self.accessory_handler.set_characteristics( - requested_chars, cast("tuple[str, int]", self.client_address) + result = self.accessory_handler._set_characteristics_for_controller( + requested_chars, + cast("tuple[str, int]", self.client_address), + RESPONSE_TIMEOUT, ) assert self.response is not None # noqa: S101 + if inspect.isawaitable(result): + # A setter is still running; answer once it is done. The driver + # times out slow setters itself. + self.response.task = asyncio.ensure_future( + self._async_set_characteristics_body(self.response, result) + ) + return self._fill_set_characteristics_response(self.response, result) def _fill_set_characteristics_response( @@ -730,6 +740,13 @@ def _fill_set_characteristics_response( response.headers.append(("Content-Type", self.JSON_RESPONSE_TYPE)) response.body = to_hap_json(result) + async def _async_set_characteristics_body( + self, response: HAPResponse, result: Awaitable[dict[str, Any] | None] + ) -> bytes: + """Wait for the pending setters, then fill in the write response.""" + self._fill_set_characteristics_response(response, await result) + return response.body + def handle_prepare(self) -> None: """Handles a client request to prepare to write.""" if not self.is_encrypted: diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index a2975924..3c539a0a 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -2,6 +2,7 @@ import asyncio from concurrent.futures import ThreadPoolExecutor +import inspect import os import tempfile import time @@ -611,7 +612,7 @@ def test_listen_address(kwargs: dict[str, Any], expected: Any) -> None: def _write( driver: AccessoryDriver, acc: Accessory, char: Characteristic, **query: Any -) -> dict[str, Any] | None: +) -> Any: """Write 0 to ``char`` from a mock client with extra query fields.""" return driver.set_characteristics( { @@ -723,6 +724,422 @@ def test_write_response_value_of_write_only_char_not_readable( assert HAP_REPR_VALUE not in acc_rep[HAP_REPR_SERVICES][-1][HAP_REPR_CHARS][0] +ASYNC_TIMEOUT = 1.0 + + +async def _controller_write( + driver: AccessoryDriver, acc: Accessory, char: Characteristic, **query: Any +) -> dict[str, Any] | None: + """Write 0 to ``char`` like a controller and wait for the async setters.""" + result = driver._set_characteristics_for_controller( + { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: acc.iid_manager.get_iid(char), + HAP_REPR_VALUE: 0, + **query, + } + ] + }, + ("1.2.3.4", 5), + ASYNC_TIMEOUT, + ) + assert inspect.isawaitable(result) + return await result + + +async def _async_return_one(value: int) -> int: + await asyncio.sleep(0) + return 1 + + +async def _async_return_one_with_addr(value: int, sender_client_addr: Any) -> int: + await asyncio.sleep(0) + return 1 + + +async def _async_return_none(value: int) -> None: + await asyncio.sleep(0) + + +async def _async_raise(value: int) -> NoReturn: + await asyncio.sleep(0) + raise ValueError("setter failed") + + +@pytest.mark.parametrize("bridged", [False, True], ids=["standalone", "bridged"]) +@pytest.mark.parametrize( + ("setter", "permissions", "expected", "stored"), + [ + pytest.param( + _async_return_one, + ["pr", "pw", "wr"], + {HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS, HAP_REPR_VALUE: 1}, + 1, + id="write-response", + ), + pytest.param( + _async_return_one_with_addr, + ["pw", "wr"], + {HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS, HAP_REPR_VALUE: 1}, + 1, + id="write-response-with-addr", + ), + pytest.param( + _async_raise, + ["pr", "pw", "wr"], + {HAP_REPR_STATUS: HAP_SERVER_STATUS.SERVICE_COMMUNICATION_FAILURE}, + 0, + id="error", + ), + ], +) +async def test_async_setter_result( + driver: AccessoryDriver, + bridged: bool, + setter: Any, + permissions: list[str], + expected: dict[str, Any], + stored: int, +) -> None: + """The result of an async setter is awaited before the write is answered.""" + acc, char, _ = _add_bridged_or_standalone(driver, permissions, bridged) + char.setter_callback = setter + + result = await _controller_write( + driver, acc, char, **{HAP_REPR_WRITE_RESPONSE: True} + ) + + assert result == { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: acc.iid_manager.get_iid(char), + **expected, + } + ] + } + assert char.value == stored + + +@pytest.mark.parametrize( + ("setter", "permissions", "query"), + [ + pytest.param(_async_return_one, ["pr", "pw", "wr"], {}, id="not-requested"), + pytest.param( + _async_return_one, + ["pr", "pw"], + {HAP_REPR_WRITE_RESPONSE: True}, + id="no-wr-permission", + ), + pytest.param( + _async_return_none, + ["pr", "pw", "wr"], + {HAP_REPR_WRITE_RESPONSE: True}, + id="no-value", + ), + ], +) +async def test_async_setter_without_write_response( + driver: AccessoryDriver, + setter: Any, + permissions: list[str], + query: dict[str, Any], +) -> None: + """A successful async setter without a write response leaves no body.""" + acc, char, _ = _add_bridged_or_standalone(driver, permissions, False) + char.setter_callback = setter + + assert await _controller_write(driver, acc, char, **query) is None + assert char.value == 0 + + +async def _async_level_setter(values: Any) -> None: + raise AssertionError("must not run") + + +@pytest.mark.parametrize( + "attach", + [ + pytest.param( + lambda acc, service: setattr(acc, "setter_callback", _async_level_setter), + id="accessory", + ), + pytest.param( + lambda acc, service: setattr( + service, "setter_callback", _async_level_setter + ), + id="service", + ), + ], +) +async def test_async_accessory_or_service_setter_is_closed( + driver: AccessoryDriver, attach: Any, caplog: pytest.LogCaptureFixture +) -> None: + """A coroutine from an accessory or service setter is closed with a warning.""" + acc = Accessory(driver, "Test Accessory") + service = Service(uuid1(), "Test Service") + char = Characteristic( + "Value", uuid1(), {PROP_FORMAT: HAP_FORMAT_INT, PROP_PERMISSIONS: ["pr", "pw"]} + ) + service.add_characteristic(char) + acc.add_service(service) + driver.add_accessory(acc) + attach(acc, service) + + result = driver._set_characteristics_for_controller( + { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: acc.iid_manager.get_iid(char), + HAP_REPR_VALUE: 2, + } + ] + }, + ("1.2.3.4", 5), + ASYNC_TIMEOUT, + ) + + assert result is None + assert char.value == 2 + assert "only characteristic setters may be coroutine functions" in caplog.text + + +async def test_async_and_sync_setters_in_one_write(driver: AccessoryDriver) -> None: + """Sync and async setters of one request share a single response.""" + acc = Accessory(driver, "Test Accessory") + service = Service(uuid1(), "Test Service") + async_char = Characteristic( + "Async", + uuid1(), + {PROP_FORMAT: HAP_FORMAT_INT, PROP_PERMISSIONS: ["pr", "pw", "wr"]}, + ) + async_char.setter_callback = _async_return_one + sync_char = Characteristic( + "Sync", uuid1(), {PROP_FORMAT: HAP_FORMAT_INT, PROP_PERMISSIONS: ["pr", "pw"]} + ) + sync_char.setter_callback = MagicMock(side_effect=ValueError) + service.add_characteristic(async_char, sync_char) + service_setter = MagicMock() + service.setter_callback = service_setter + acc.add_service(service) + driver.add_accessory(acc) + async_iid = acc.iid_manager.get_iid(async_char) + sync_iid = acc.iid_manager.get_iid(sync_char) + + result = driver._set_characteristics_for_controller( + { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: async_iid, + HAP_REPR_VALUE: 3, + HAP_REPR_WRITE_RESPONSE: True, + }, + {HAP_REPR_AID: acc.aid, HAP_REPR_IID: sync_iid, HAP_REPR_VALUE: 4}, + ] + }, + ("1.2.3.4", 5), + ASYNC_TIMEOUT, + ) + + service_setter.assert_called_once_with({"Async": 3, "Sync": 4}) + assert inspect.isawaitable(result) + assert await result == { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: async_iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS, + HAP_REPR_VALUE: 1, + }, + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: sync_iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.SERVICE_COMMUNICATION_FAILURE, + }, + ] + } + + +class _SlowSetter: + """An async setter that runs until it is cancelled.""" + + def __init__(self) -> None: + self.started = asyncio.Event() + self.cancelled = asyncio.Event() + + async def __call__(self, value: int) -> None: + self.started.set() + try: + await asyncio.sleep(10) + except asyncio.CancelledError: + self.cancelled.set() + raise + + +class _RecordingSetter: + """An async setter that records that it ran and answers 1.""" + + def __init__(self) -> None: + self.ran = asyncio.Event() + + async def __call__(self, value: int) -> int: + self.ran.set() + return 1 + + +async def test_async_setter_timeout_keeps_other_results( + driver: AccessoryDriver, +) -> None: + """Only the setter that runs too long answers -70408, and it is cancelled.""" + acc = Accessory(driver, "Test Accessory") + service = Service(uuid1(), "Test Service") + slow = Characteristic( + "Slow", uuid1(), {PROP_FORMAT: HAP_FORMAT_INT, PROP_PERMISSIONS: ["pw"]} + ) + slow_setter = _SlowSetter() + slow.setter_callback = slow_setter + fast = Characteristic( + "Fast", uuid1(), {PROP_FORMAT: HAP_FORMAT_INT, PROP_PERMISSIONS: ["pw", "wr"]} + ) + fast.setter_callback = _async_return_one + service.add_characteristic(slow, fast) + acc.add_service(service) + driver.add_accessory(acc) + slow_iid = acc.iid_manager.get_iid(slow) + fast_iid = acc.iid_manager.get_iid(fast) + + result = driver._set_characteristics_for_controller( + { + HAP_REPR_CHARS: [ + {HAP_REPR_AID: acc.aid, HAP_REPR_IID: slow_iid, HAP_REPR_VALUE: 1}, + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: fast_iid, + HAP_REPR_VALUE: 2, + HAP_REPR_WRITE_RESPONSE: True, + }, + ] + }, + ("1.2.3.4", 5), + 0.05, + ) + + assert inspect.isawaitable(result) + assert await result == { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: slow_iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.OPERATION_TIMED_OUT, + }, + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: fast_iid, + HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS, + HAP_REPR_VALUE: 1, + }, + ] + } + await asyncio.wait_for(slow_setter.cancelled.wait(), ASYNC_TIMEOUT) + + +async def test_cancelled_write_cancels_async_setters( + driver: AccessoryDriver, +) -> None: + """Cancelling the pending write, e.g. on a closed connection, stops setters.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pw"], False) + setter = _SlowSetter() + char.setter_callback = setter + task = asyncio.ensure_future(_controller_write(driver, acc, char)) + await setter.started.wait() + + task.cancel() + + with pytest.raises(asyncio.CancelledError): + await task + await asyncio.wait_for(setter.cancelled.wait(), ASYNC_TIMEOUT) + + +class _CoroutineRecorder: + """A setter that hands out a coroutine and keeps a reference to it.""" + + def __init__(self) -> None: + self.coro: Any = None + + def __call__(self, value: int) -> Any: + self.coro = _async_return_none(value) + return self.coro + + +async def test_malformed_entry_closes_pending_setters(driver: AccessoryDriver) -> None: + """A write that fails on a later entry closes the coroutines it created.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pw"], False) + setter = _CoroutineRecorder() + char.setter_callback = setter + + with pytest.raises(KeyError): + driver._set_characteristics_for_controller( + { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: acc.iid_manager.get_iid(char), + HAP_REPR_VALUE: 0, + }, + {HAP_REPR_AID: acc.aid}, + ] + }, + ("1.2.3.4", 5), + ASYNC_TIMEOUT, + ) + + assert inspect.getcoroutinestate(setter.coro) == inspect.CORO_CLOSED + + +async def test_public_set_characteristics_schedules_async_setters( + driver: AccessoryDriver, +) -> None: + """Called directly, the write is answered at once and the setter runs later.""" + driver.loop = asyncio.get_running_loop() + acc, char, _ = _add_bridged_or_standalone(driver, ["pw", "wr"], False) + setter = _RecordingSetter() + char.setter_callback = setter + + assert _write(driver, acc, char, **{HAP_REPR_WRITE_RESPONSE: True}) is None + await asyncio.wait_for(setter.ran.wait(), ASYNC_TIMEOUT) + + +async def test_setter_returning_a_task_is_not_awaited( + driver: AccessoryDriver, +) -> None: + """A sync setter that starts a task is answered without waiting for it.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pw"], False) + task = asyncio.ensure_future(asyncio.sleep(10)) + char.setter_callback = MagicMock(return_value=task) + + result = driver._set_characteristics_for_controller( + { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: acc.iid_manager.get_iid(char), + HAP_REPR_VALUE: 0, + } + ] + }, + ("1.2.3.4", 5), + ASYNC_TIMEOUT, + ) + + assert result is None + assert not task.done() + task.cancel() + + def test_service_callbacks(driver: AccessoryDriver) -> None: bridge = Bridge(driver, "mybridge") acc = Accessory(driver, "TestAcc", aid=2) diff --git a/tests/test_hap_handler.py b/tests/test_hap_handler.py index 9dc91435..a45e96ee 100644 --- a/tests/test_hap_handler.py +++ b/tests/test_hap_handler.py @@ -1,10 +1,11 @@ """Tests for the HAPServerHandler.""" +import asyncio import json from typing import Any, NoReturn from unittest.mock import patch from urllib.parse import urlparse -from uuid import UUID +from uuid import UUID, uuid1 from chacha20poly1305_reuseable import ChaCha20Poly1305Reusable as ChaCha20Poly1305 from cryptography.hazmat.primitives import serialization @@ -18,8 +19,9 @@ MAX_FAILED_PAIRING_ATTEMPTS, AccessoryDriver, ) -from pyhap.characteristic import CharacteristicError +from pyhap.characteristic import Characteristic, CharacteristicError from pyhap.const import HAP_PERMISSIONS, HAP_SERVER_STATUS +from pyhap.service import Service CLIENT_UUID = UUID("7d0d1ee9-46fe-4a56-a115-69df3f6860c1") CLIENT_UUID_BYTES = str(CLIENT_UUID).upper().encode("utf-8") @@ -1194,3 +1196,85 @@ def test_invalid_pairing_three(driver: AccessoryDriver) -> None: hap_handler.HAP_TLV_TAGS.SEQUENCE_NUM: hap_handler.HAP_TLV_STATES.M6, hap_handler.HAP_TLV_TAGS.ERROR_CODE: hap_handler.HAP_TLV_ERRORS.AUTHENTICATION, } + + +def _dispatch_write_to_async_setter( + driver: AccessoryDriver, setter: Any, body: bytes +) -> hap_handler.HAPResponse: + """Dispatch a characteristics write to a wr characteristic with ``setter``.""" + acc = Accessory(driver, "TestAcc") + service = Service(uuid1(), "Test Service") + char = Characteristic( + "Value", uuid1(), {"Format": "int", "Permissions": ["pr", "pw", "wr"]} + ) + char.setter_callback = setter + service.add_characteristic(char) + acc.add_service(service) + driver.add_accessory(acc) + assert acc.iid_manager.get_iid(char) == 9 + + handler = hap_handler.HAPServerHandler(driver, "peername") + handler.is_encrypted = True + request = h11.Request( + method="PUT", target="/characteristics", headers=[(b"host", b"t")] + ) + return handler.dispatch(request, body) + + +async def _async_return_one(_value: int) -> int: + await asyncio.sleep(0) + return 1 + + +async def _async_sleep(_value: int) -> None: + await asyncio.sleep(10) + + +@pytest.mark.parametrize( + ("body", "status_code", "headers", "response_body"), + [ + pytest.param( + b'{"characteristics":[{"aid":1,"iid":9,"value":0,"r":true}]}', + 207, + [("Content-Type", "application/hap+json")], + b'{"characteristics":[{"status":0,"value":1,"aid":1,"iid":9}]}', + id="write-response", + ), + pytest.param( + b'{"characteristics":[{"aid":1,"iid":9,"value":0}]}', + 204, + [], + b"", + id="no-write-response", + ), + ], +) +async def test_handle_set_characteristics_async_setter( + driver: AccessoryDriver, + body: bytes, + status_code: int, + headers: list[tuple[str, str]], + response_body: bytes, +) -> None: + """A write to an async setter is answered once the setter is done.""" + response = _dispatch_write_to_async_setter(driver, _async_return_one, body) + + assert response.task is not None + assert await response.task == response_body + assert response.status_code == status_code + assert response.headers == headers + + +async def test_handle_set_characteristics_async_setter_timeout( + driver: AccessoryDriver, +) -> None: + """An async setter that does not finish in time answers -70408 for its char.""" + with patch.object(hap_handler, "RESPONSE_TIMEOUT", 0.01): + response = _dispatch_write_to_async_setter( + driver, _async_sleep, b'{"characteristics":[{"aid":1,"iid":9,"value":0}]}' + ) + assert response.task is not None + assert await response.task == ( + b'{"characteristics":[{"status":-70408,"aid":1,"iid":9}]}' + ) + assert response.status_code == 207 diff --git a/tests/test_hap_protocol.py b/tests/test_hap_protocol.py index 749e5b00..d1efbc4b 100644 --- a/tests/test_hap_protocol.py +++ b/tests/test_hap_protocol.py @@ -5,15 +5,18 @@ import time from typing import Any, NoReturn, cast from unittest.mock import MagicMock, Mock, patch +from uuid import uuid1 from cryptography.exceptions import InvalidTag from pyhap import hap_handler, hap_protocol from pyhap.accessory import Accessory, Bridge, get_topic from pyhap.accessory_driver import AccessoryDriver +from pyhap.characteristic import Characteristic from pyhap.const import HAP_REPR_AID, HAP_REPR_IID, HAP_REPR_VALUE from pyhap.hap_crypto import HAPCrypto, hap_hkdf from pyhap.hap_handler import HAPResponse +from pyhap.service import Service class MockTransport(asyncio.Transport): @@ -925,6 +928,98 @@ async def _async_get_snapshot(*_: Any) -> bytes: hap_proto.close() +ASYNC_PUT_REQUEST = ( + b"PUT /characteristics HTTP/1.1\r\n" + b"Host: HASS12\\032AD1C22._hap._tcp.local\r\n" + b"Content-Length: 58\r\n" + b"Content-Type: application/hap+json\r\n" + b"\r\n" + b'{"characteristics":[{"aid":1,"iid":9,"value":0,"r":true}]}' +) +ASYNC_PUT_RESPONSE = ( + b"HTTP/1.1 207 Multi-Status\r\n" + b"Content-Type: application/hap+json\r\n" + b"Content-Length: 60\r\n\r\n" + b'{"characteristics":[{"status":0,"value":1,"aid":1,"iid":9}]}' +) + + +def _async_setter_protocol( + driver: AccessoryDriver, setter_done: asyncio.Event +) -> hap_protocol.HAPServerProtocol: + """A connection to an accessory whose char 1.9 has an async setter. + + The setter waits for ``setter_done`` and answers 1 in the write response. + """ + transport = MagicMock() + transport.is_closing = Mock(return_value=False) + + async def _setter(_value: int) -> int: + await setter_done.wait() + return 1 + + acc = Accessory(driver, "TestAcc") + service = Service(uuid1(), "Test Service") + char = Characteristic( + "Value", uuid1(), {"Format": "int", "Permissions": ["pr", "pw", "wr"]} + ) + char.setter_callback = _setter + service.add_characteristic(char) + acc.add_service(service) + driver.add_accessory(acc) + assert acc.iid_manager.get_iid(char) == 9 + + hap_proto = hap_protocol.HAPServerProtocol(MagicMock(), {}, driver) + hap_proto.connection_made(transport) + _enable_encryption(hap_proto) + return hap_proto + + +async def test_set_characteristics_async_setter_write_response( + driver: AccessoryDriver, +) -> None: + """The write response of an async setter is sent once the setter is done.""" + setter_done = asyncio.Event() + hap_proto = _async_setter_protocol(driver, setter_done) + + with patch.object(hap_proto.transport, "writelines") as writelines: + hap_proto.data_received(ASYNC_PUT_REQUEST) + writelines.assert_not_called() + setter_done.set() + await _await_response(hap_proto) + + hap_proto.close() + assert b"".join(writelines.call_args_list[0][0]) == ASYNC_PUT_RESPONSE + + +async def test_get_pipelined_behind_async_setter(driver: AccessoryDriver) -> None: + """A GET behind an async write is answered after the 207, with the new value.""" + setter_done = asyncio.Event() + hap_proto = _async_setter_protocol(driver, setter_done) + + with patch.object(hap_proto.transport, "writelines") as writelines: + hap_proto.data_received( + ASYNC_PUT_REQUEST + + b"GET /characteristics?id=1.9 HTTP/1.1\r\n" + + b"Host: HASS12\\032AD1C22._hap._tcp.local\r\n\r\n" + ) + await asyncio.sleep(0) + writelines.assert_not_called() + setter_done.set() + await _await_response(hap_proto) + await asyncio.sleep(0) + + hap_proto.close() + assert writelines.call_count == 2 + assert b"".join(writelines.call_args_list[0][0]) == ASYNC_PUT_RESPONSE + assert b"".join(writelines.call_args_list[1][0]) == ( + b"HTTP/1.1 200 OK\r\n" + b"Content-Type: application/hap+json\r\n" + b"Content-Length: 49\r\n\r\n" + b'{"characteristics":[{"aid":1,"iid":9,"value":1}]}' + ) + + def test_upgrade_to_encrypted(driver: AccessoryDriver) -> None: """Test we switch to encrypted wen we get a shared_key.""" loop = MagicMock() From f946d1f3fa32750bb4857450aac4278ce63d48a7 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Sun, 8 Nov 2026 21:58:12 +0100 Subject: [PATCH 09/21] Only let the writing connection read back a write response A write response of a characteristic that also has pr was stored as its value, so any paired controller could read it with GET /characteristics or /accessories. Keep it per connection instead: the connection that wrote reads its last response until it closes. Other connections read the characteristic's own value, which the write leaves as it was, also when the setter raises or returns nothing. Any new write of a connection drops its former response. Characteristics without pr keep the response as the value. --- pyhap/accessory_driver.py | 106 +++++++++++--- pyhap/hap_handler.py | 2 +- tests/test_accessory_driver.py | 257 +++++++++++++++++++++++++++++++-- tests/test_hap_handler.py | 56 ++++++- 4 files changed, 385 insertions(+), 36 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index bd05a8c2..88379cbe 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -88,22 +88,12 @@ MAX_PREPARED_WRITE_TTL_SECONDS = 30.0 -def _store_write_response(char: Characteristic, response: Any) -> Any: - """Keep the write response as the value and return what to send. - - The written value of a write response characteristic, such as an SDP - offer, is a request. What the accessory answered is its state. - """ +def _stored_value(char: Any, value: Any) -> Any: + """Return the value ``client_update_value`` stores for ``value``.""" try: - char.value = char.to_valid_value(response) + return char.to_valid_value(value) except ValueError: - logger.debug( - "%s: keeping the written value, the write response is invalid: %s", - char.display_name, - response, - ) - return response - return char.value + return value def _wrap_char_setter(char: Any, value: Any, client_addr: Any) -> tuple[int, Any]: @@ -356,6 +346,8 @@ def __init__( network_tuple = (self._listen_address, self.state.port) self.http_server = HAPServer(network_tuple, self) self.prepared_writes = {} + # Write responses of readable characteristics, per client. + self._write_responses: dict[tuple[str, int], dict[Characteristic, Any]] = {} def listen_address(self) -> str | list[str]: """Return the address, or list of addresses, the HAP server listens on.""" @@ -602,6 +594,8 @@ def connection_lost(self, client: Any) -> None: for topic in client_topics: self.async_subscribe_client_topic(client, topic, subscribe=False) self.prepared_writes.pop(client, None) + # Deferred, so a write response that is already on its way is dropped too. + self.loop.call_soon(self._write_responses.pop, client, None) def publish( self, @@ -926,12 +920,18 @@ def get_accessories(self, include_value: bool = True) -> dict[str, Any]: logger.debug("Get accessories response: %s", hap_rep) return {HAP_REPR_ACCS: hap_rep} - def get_characteristics(self, char_ids: Any) -> dict[str, Any]: + def get_characteristics( + self, char_ids: Any, client_addr: tuple[str, int] | None = None + ) -> dict[str, Any]: """Returns values for the required characteristics. :param char_ids: A list of characteristic "paths", e.g. "1.2" is aid 1, iid 2. :type char_ids: list + :param client_addr: The client that reads. It reads back its own write + responses. + :type client_addr: tuple + :return: Status success for each required characteristic. For example: .. code-block:: python @@ -947,6 +947,9 @@ def get_characteristics(self, char_ids: Any) -> dict[str, Any]: :rtype: dict """ assert self.accessory is not None # noqa: S101 + write_responses = ( + self._write_responses.get(client_addr, {}) if client_addr else {} + ) chars = [] for aid_iid in char_ids: aid, iid = (int(i) for i in aid_iid.split(".")) @@ -984,7 +987,11 @@ def get_characteristics(self, char_ids: Any) -> dict[str, Any]: HAP_SERVER_STATUS.WRITE_ONLY_CHARACTERISTIC ) elif available: - rep[HAP_REPR_VALUE] = char.get_value() + rep[HAP_REPR_VALUE] = ( + write_responses[char] + if char in write_responses + else char.get_value() + ) rep[HAP_REPR_STATUS] = HAP_SERVER_STATUS.SUCCESS except CharacteristicError: logger.error( @@ -1103,6 +1110,24 @@ def set_characteristics( } continue + # Only characteristics with the write response permission answer + # a write with a value, and only when the controller asks for it. + write_response = bool( + query.get(HAP_REPR_WRITE_RESPONSE, False) + and HAP_PERMISSION_WRITE_RESPONSE in char.properties[PROP_PERMISSIONS] + ) + # A new write replaces a former response of this connection. The + # request of a readable write response characteristic is not + # shared, whatever the setter does, except to ev subscribers. + if value is not None: + self._write_responses.get(client_addr, {}).pop(char, None) + private_write = ( + write_response + and value is not None + and HAP_PERMISSION_READ in char.properties[PROP_PERMISSIONS] + ) + previous_value = char.value + set_result = HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST set_result_value = None @@ -1110,17 +1135,16 @@ def set_characteristics( set_result, set_result_value = _wrap_char_setter( char, value, client_addr ) + # Put back the former value unless the setter set its own. + if private_write and char.value == _stored_value(char, value): + char.value = previous_value - # Only characteristics with the write response permission answer - # a write with a value, and only when the controller asks for it. - if ( - set_result_value is not None - and query.get(HAP_REPR_WRITE_RESPONSE, False) - and HAP_PERMISSION_WRITE_RESPONSE in char.properties[PROP_PERMISSIONS] - ): + if set_result_value is not None and write_response: result = { HAP_REPR_STATUS: set_result, - HAP_REPR_VALUE: _store_write_response(char, set_result_value), + HAP_REPR_VALUE: self._store_write_response( + char, set_result_value, client_addr + ), } else: result = {HAP_REPR_STATUS: set_result} @@ -1170,6 +1194,40 @@ def set_characteristics( return {HAP_REPR_CHARS: characteristics} if nonempty_results_exist else None + def _store_write_response( + self, + char: Characteristic, + response: Any, + client_addr: tuple[str, int] | None, + ) -> Any: + """Keep the write response and return what to send. + + The written value of a write response characteristic, such as an SDP + offer, is a request. What the accessory answered is its state. If the + characteristic is readable, only the connection that wrote reads the + response back, until it closes, and it wins over the getter and a later + ``set_value``. Other connections read the characteristic's own value, + which the write leaves as it was. A write without ``r`` stays readable + for everyone, and a characteristic that also has ``ev`` notifies the + written value as any write does. + """ + readable = HAP_PERMISSION_READ in char.properties[PROP_PERMISSIONS] + try: + response = char.to_valid_value(response) + except ValueError: + logger.debug( + "%s: not keeping the write response, it is invalid: %s", + char.display_name, + response, + ) + return response + if readable: + if client_addr is not None: + self._write_responses.setdefault(client_addr, {})[char] = response + else: + char.value = response + return response + def prepare( self, prepare_query: dict[str, Any], client_addr: tuple[str, int] ) -> dict[str, int]: diff --git a/pyhap/hap_handler.py b/pyhap/hap_handler.py index 83a47380..e027811a 100644 --- a/pyhap/hap_handler.py +++ b/pyhap/hap_handler.py @@ -679,7 +679,7 @@ def handle_get_characteristics(self) -> None: assert self.parsed_url is not None # noqa: S101 params = parse_qs(self.parsed_url.query) response = self.accessory_handler.get_characteristics( - params["id"][0].split(",") + params["id"][0].split(","), cast("tuple[str, int]", self.client_address) ) chars = response[HAP_REPR_CHARS] diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index 475a3c7f..bc85ddb2 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -2,6 +2,7 @@ import asyncio from concurrent.futures import ThreadPoolExecutor +from functools import partial import os import tempfile import time @@ -609,6 +610,10 @@ def test_listen_address(kwargs: dict[str, Any], expected: Any) -> None: assert server.call_args.args[0] == (expected, 51234) +WRITER = ("1.2.3.4", 5) +OTHER_CONTROLLER = ("5.6.7.8", 9) + + def _write( driver: AccessoryDriver, acc: Accessory, char: Characteristic, **query: Any ) -> dict[str, Any] | None: @@ -624,17 +629,25 @@ def _write( } ] }, - ("1.2.3.4", 5), + WRITER, ) @pytest.mark.parametrize("bridged", [False, True], ids=["standalone", "bridged"]) -@pytest.mark.parametrize("permissions", [["pr", "pw", "wr"], ["pw", "wr"]]) +@pytest.mark.parametrize( + ("permissions", "stored"), + [ + # A readable char keeps its default value, see the read tests below. + pytest.param(["pr", "pw", "wr"], 0, id="readable"), + pytest.param(["pw", "wr"], 1, id="write-only"), + ], +) @pytest.mark.parametrize(("returned", "sent"), [(1, 1), (1.7, 1)]) def test_write_response_returned_when_requested( driver: AccessoryDriver, bridged: bool, permissions: list[str], + stored: int, returned: Any, sent: int, ) -> None: @@ -659,8 +672,7 @@ def test_write_response_returned_when_requested( } char.setter_callback.assert_called_once_with(0) service_setter.assert_called_once_with({"Value": 0}) - # Like HAP-NodeJS, the response is the value from now on. - assert char.value == sent + assert char.value == stored @pytest.mark.parametrize( @@ -688,16 +700,27 @@ def test_write_response_not_returned( assert char.value == 0 -def test_invalid_write_response_keeps_written_value(driver: AccessoryDriver) -> None: - """A response the char can't hold is still sent, the written value stays.""" - acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "wr"], False) +@pytest.mark.parametrize( + ("permissions", "stored"), + [ + pytest.param(["pr", "pw", "wr"], 0, id="readable"), + pytest.param(["pw", "wr"], 5, id="write-only"), + ], +) +def test_invalid_write_response_is_sent_but_not_kept( + driver: AccessoryDriver, permissions: list[str], stored: int +) -> None: + """A response the char can't hold is still sent, but not kept as its value.""" + acc, char, _ = _add_bridged_or_standalone(driver, permissions, False) char.setter_callback = MagicMock(return_value="not a number") - response = _write(driver, acc, char, **{HAP_REPR_WRITE_RESPONSE: True}) + response = _write( + driver, acc, char, **{HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: True} + ) assert response is not None assert response[HAP_REPR_CHARS][0][HAP_REPR_VALUE] == "not a number" - assert char.value == 0 + assert char.value == stored def test_write_response_value_of_write_only_char_not_readable( @@ -723,6 +746,222 @@ def test_write_response_value_of_write_only_char_not_readable( assert HAP_REPR_VALUE not in acc_rep[HAP_REPR_SERVICES][-1][HAP_REPR_CHARS][0] +def _add_write_response_char( + driver: AccessoryDriver, bridged: bool +) -> tuple[Accessory, Characteristic, str]: + """Add a pr, pw and wr char that answers a write with 7.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "wr"], bridged) + char.getter_callback = None + char.setter_callback = MagicMock(return_value=7) + return acc, char, f"{acc.aid}.{acc.iid_manager.get_iid(char)}" + + +def _read(driver: AccessoryDriver, char_id: str, client_addr: Any) -> Any: + """Return the value ``client_addr`` reads from ``char_id``.""" + (rep,) = driver.get_characteristics([char_id], client_addr)[HAP_REPR_CHARS] + assert rep[HAP_REPR_STATUS] == HAP_SERVER_STATUS.SUCCESS + return rep[HAP_REPR_VALUE] + + +@pytest.mark.parametrize("bridged", [False, True], ids=["standalone", "bridged"]) +@pytest.mark.parametrize( + ("reader", "expected"), + [ + pytest.param(WRITER, 7, id="writer"), + pytest.param(OTHER_CONTROLLER, 0, id="other-controller"), + pytest.param(None, 0, id="no-client"), + ], +) +def test_write_response_readable_only_by_writer( + driver: AccessoryDriver, bridged: bool, reader: Any, expected: int +) -> None: + """Only the controller that wrote reads its write response back.""" + acc, char, char_id = _add_write_response_char(driver, bridged) + + _write(driver, acc, char, **{HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: True}) + + assert _read(driver, char_id, reader) == expected + + +def test_write_responses_kept_per_controller(driver: AccessoryDriver) -> None: + """Each controller reads its own last write response.""" + acc, char, char_id = _add_write_response_char(driver, False) + char.setter_callback = MagicMock(side_effect=[7, 8]) + request = { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: acc.iid_manager.get_iid(char), + HAP_REPR_VALUE: 5, + HAP_REPR_WRITE_RESPONSE: True, + } + ] + } + + driver.set_characteristics(request, WRITER) + driver.set_characteristics(request, OTHER_CONTROLLER) + + assert _read(driver, char_id, WRITER) == 7 + assert _read(driver, char_id, OTHER_CONTROLLER) == 8 + + +@pytest.mark.parametrize("bridged", [False, True], ids=["standalone", "bridged"]) +def test_write_response_not_in_accessories( + driver: AccessoryDriver, bridged: bool +) -> None: + """The accessory database has the default value, not the write response.""" + acc, char, _ = _add_write_response_char(driver, bridged) + + _write(driver, acc, char, **{HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: True}) + + acc_rep = driver.get_accessories()[HAP_REPR_ACCS][-1] + assert acc_rep[HAP_REPR_SERVICES][-1][HAP_REPR_CHARS][0][HAP_REPR_VALUE] == 0 + + +async def test_write_response_dropped_on_disconnect(driver: AccessoryDriver) -> None: + """A write response is not read back after the connection closed.""" + driver.loop = asyncio.get_running_loop() + acc, char, char_id = _add_write_response_char(driver, False) + _write(driver, acc, char, **{HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: True}) + + driver.connection_lost(WRITER) + assert _read(driver, char_id, WRITER) == 7 + # The response is dropped once the loop runs. + await asyncio.sleep(0) + + assert _read(driver, char_id, WRITER) == 0 + + +def test_plain_write_replaces_write_response(driver: AccessoryDriver) -> None: + """A write without ``r`` drops the writer's former response.""" + acc, char, char_id = _add_write_response_char(driver, False) + _write(driver, acc, char, **{HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: True}) + + _write(driver, acc, char, **{HAP_REPR_VALUE: 9}) + + assert _read(driver, char_id, WRITER) == 9 + assert _read(driver, char_id, OTHER_CONTROLLER) == 9 + + +@pytest.mark.parametrize( + ("setter_kwargs", "writer_reads"), + [ + pytest.param({"return_value": 7}, 7, id="answered"), + pytest.param({"side_effect": RuntimeError}, 3, id="raises"), + ], +) +def test_write_response_keeps_accessory_value( + driver: AccessoryDriver, setter_kwargs: dict[str, Any], writer_reads: int +) -> None: + """Other connections keep reading the value the accessory set.""" + acc, char, char_id = _add_write_response_char(driver, False) + char.setter_callback = MagicMock(**setter_kwargs) + char.set_value(3) + + _write(driver, acc, char, **{HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: True}) + + assert _read(driver, char_id, WRITER) == writer_reads + assert _read(driver, char_id, OTHER_CONTROLLER) == 3 + + +def test_write_response_keeps_value_set_by_setter(driver: AccessoryDriver) -> None: + """A value the setter sets on its own characteristic is kept.""" + acc, char, char_id = _add_write_response_char(driver, False) + + def _setter(value: int) -> int: + char.set_value(42) + return 7 + + char.setter_callback = _setter + + _write(driver, acc, char, **{HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: True}) + + assert _read(driver, char_id, WRITER) == 7 + assert _read(driver, char_id, OTHER_CONTROLLER) == 42 + + +@pytest.mark.parametrize("reader", [WRITER, OTHER_CONTROLLER], ids=["writer", "other"]) +@pytest.mark.parametrize( + "make_setter", + [ + pytest.param(partial(MagicMock, side_effect=RuntimeError), id="raises"), + pytest.param(partial(MagicMock, return_value=None), id="no-response"), + pytest.param(lambda: None, id="no-setter"), + ], +) +def test_write_response_request_not_shared_without_response( + driver: AccessoryDriver, reader: Any, make_setter: Any +) -> None: + """The written request stays private when no response is kept.""" + acc, char, char_id = _add_write_response_char(driver, False) + char.setter_callback = make_setter() + + _write(driver, acc, char, **{HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: True}) + + assert _read(driver, char_id, reader) == 0 + + +@pytest.mark.parametrize( + "second", + [ + pytest.param(RuntimeError("boom"), id="raises"), + pytest.param("not a number", id="invalid"), + pytest.param(None, id="no-response"), + ], +) +def test_stale_write_response_dropped(driver: AccessoryDriver, second: Any) -> None: + """A later write that keeps no response drops the former one.""" + acc, char, char_id = _add_write_response_char(driver, False) + char.setter_callback = MagicMock(side_effect=[7, second]) + query = {HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: True} + + _write(driver, acc, char, **query) + _write(driver, acc, char, **query) + + assert _read(driver, char_id, WRITER) == 0 + + +def test_write_response_without_client_not_kept(driver: AccessoryDriver) -> None: + """A write response without a known connection is sent but not kept.""" + acc, char, _ = _add_write_response_char(driver, False) + request = { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: acc.iid_manager.get_iid(char), + HAP_REPR_VALUE: 5, + HAP_REPR_WRITE_RESPONSE: True, + } + ] + } + + no_client: Any = None + response = driver.set_characteristics(request, no_client) + + assert response is not None + assert response[HAP_REPR_CHARS][0][HAP_REPR_VALUE] == 7 + assert driver._write_responses == {} + + +@pytest.mark.parametrize("reader", [WRITER, OTHER_CONTROLLER], ids=["writer", "other"]) +@pytest.mark.parametrize( + "query", + [ + pytest.param({}, id="not-requested"), + pytest.param({HAP_REPR_WRITE_RESPONSE: False}, id="declined"), + ], +) +def test_written_value_readable_without_write_response( + driver: AccessoryDriver, reader: Any, query: dict[str, Any] +) -> None: + """Without a write response the written value is read by every controller.""" + acc, char, char_id = _add_write_response_char(driver, False) + + _write(driver, acc, char, **{HAP_REPR_VALUE: 5, **query}) + + assert _read(driver, char_id, reader) == 5 + + def test_service_callbacks(driver: AccessoryDriver) -> None: bridge = Bridge(driver, "mybridge") acc = Accessory(driver, "TestAcc", aid=2) diff --git a/tests/test_hap_handler.py b/tests/test_hap_handler.py index 9dc91435..fc390c95 100644 --- a/tests/test_hap_handler.py +++ b/tests/test_hap_handler.py @@ -4,7 +4,7 @@ from typing import Any, NoReturn from unittest.mock import patch from urllib.parse import urlparse -from uuid import UUID +from uuid import UUID, uuid1 from chacha20poly1305_reuseable import ChaCha20Poly1305Reusable as ChaCha20Poly1305 from cryptography.hazmat.primitives import serialization @@ -18,8 +18,15 @@ MAX_FAILED_PAIRING_ATTEMPTS, AccessoryDriver, ) -from pyhap.characteristic import CharacteristicError +from pyhap.characteristic import ( + HAP_FORMAT_INT, + PROP_FORMAT, + PROP_PERMISSIONS, + Characteristic, + CharacteristicError, +) from pyhap.const import HAP_PERMISSIONS, HAP_SERVER_STATUS +from pyhap.service import Service CLIENT_UUID = UUID("7d0d1ee9-46fe-4a56-a115-69df3f6860c1") CLIENT_UUID_BYTES = str(CLIENT_UUID).upper().encode("utf-8") @@ -1018,6 +1025,51 @@ def test_handle_get_characteristics_encrypted(driver: AccessoryDriver) -> None: assert decoded_response["characteristics"][0]["status"] == -70402 +@pytest.mark.parametrize( + ("reader", "expected"), + [ + pytest.param(("1.2.3.4", 5), b'"value":7', id="writer"), + pytest.param(("5.6.7.8", 9), b'"value":0', id="other-controller"), + ], +) +def test_handle_get_characteristics_write_response_per_client( + driver: AccessoryDriver, reader: tuple[str, int], expected: bytes +) -> None: + """Only the client that wrote reads the write response back.""" + acc = Accessory(driver, "TestAcc", aid=1) + service = Service(uuid1(), "Test Service") + char = Characteristic( + "Value", + uuid1(), + {PROP_FORMAT: HAP_FORMAT_INT, PROP_PERMISSIONS: ["pr", "pw", "wr"]}, + ) + char.setter_callback = lambda _value: 7 + service.add_characteristic(char) + acc.add_service(service) + driver.add_accessory(acc) + iid = acc.iid_manager.get_iid(char) + + writer = hap_handler.HAPServerHandler(driver, ("1.2.3.4", 5)) + writer.is_encrypted = True + writer.response = hap_handler.HAPResponse() + writer.request_body = ( + b'{"characteristics":[{"aid":1,"iid":%d,"value":5,"r":true}]}' % iid + ) + writer.handle_set_characteristics() + assert b'"value":7' in writer.response.body + + handler = hap_handler.HAPServerHandler(driver, reader) + handler.is_encrypted = True + response = hap_handler.HAPResponse() + handler.response = response + handler.path = f"/characteristics?id=1.{iid}" + handler.parsed_url = urlparse(handler.path) + handler.handle_get_characteristics() + + assert response.status_code == 200 + assert expected in response.body + + def test_handle_resource_unencrypted_rejected(driver: AccessoryDriver) -> None: """Verify an unencrypted POST /resource is rejected with 401. From c4c842b2a303bf245f25c257256a522829bfbf33 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Sun, 8 Nov 2026 22:15:14 +0100 Subject: [PATCH 10/21] Test write response privacy with async setters The writing connection reads an async setter's response back, other connections don't, the request stays private while the setter runs, and a response that completes as the connection closes is not kept. --- tests/test_accessory_driver.py | 83 ++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index 8c44f35e..62cafbd4 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -846,6 +846,89 @@ async def test_async_setter_result( assert char.value == stored +@pytest.mark.parametrize( + ("reader", "expected"), + [ + pytest.param(WRITER, 1, id="writer"), + pytest.param(OTHER_CONTROLLER, 0, id="other-controller"), + ], +) +async def test_async_write_response_readable_only_by_writer( + driver: AccessoryDriver, reader: tuple[str, int], expected: int +) -> None: + """Only the controller that wrote reads an async setter's response back.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "wr"], False) + char.getter_callback = None + char.setter_callback = _async_return_one + + await _controller_write( + driver, acc, char, **{HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: True} + ) + + char_id = f"{acc.aid}.{acc.iid_manager.get_iid(char)}" + assert _read(driver, char_id, reader) == expected + + +async def test_async_write_response_dropped_when_closed_while_queued( + driver: AccessoryDriver, +) -> None: + """A response that completes as the connection closes is not kept.""" + # This covers a completion that is already queued when the connection + # closes. The cancel in ``HAPServerProtocol.close()`` covers the rest. + driver.loop = asyncio.get_running_loop() + acc, char, _ = _add_write_response_char(driver, False) + release = asyncio.Event() + + async def _setter(value: int) -> int: + await release.wait() + return 7 + + char.setter_callback = _setter + write = asyncio.ensure_future( + _controller_write( + driver, acc, char, **{HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: True} + ) + ) + await asyncio.sleep(0) + release.set() + # Two loop steps: the setter is done and the write's completion is queued. + await asyncio.sleep(0) + await asyncio.sleep(0) + + driver.connection_lost(WRITER) + await write + await asyncio.sleep(0) + + assert driver._write_responses == {} + + +async def test_async_write_response_request_not_shared( + driver: AccessoryDriver, +) -> None: + """Other controllers don't read the written request while the setter runs.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "wr"], False) + char.getter_callback = None + release = asyncio.Event() + + async def _setter(value: int) -> int: + await release.wait() + return 1 + + char.setter_callback = _setter + write = asyncio.ensure_future( + _controller_write( + driver, acc, char, **{HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: True} + ) + ) + await asyncio.sleep(0) + + char_id = f"{acc.aid}.{acc.iid_manager.get_iid(char)}" + assert _read(driver, char_id, OTHER_CONTROLLER) == 0 + release.set() + await write + assert _read(driver, char_id, OTHER_CONTROLLER) == 0 + + @pytest.mark.parametrize( ("setter", "permissions", "query"), [ From 8f298c6b8b7149e7d7a4a15baad7c7f8fcaf18ec Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:31:11 +0100 Subject: [PATCH 11/21] Drop the redundant bool() around the write response check --- pyhap/accessory_driver.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index 88379cbe..bef8e269 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -1112,7 +1112,7 @@ def set_characteristics( # Only characteristics with the write response permission answer # a write with a value, and only when the controller asks for it. - write_response = bool( + write_response = ( query.get(HAP_REPR_WRITE_RESPONSE, False) and HAP_PERMISSION_WRITE_RESPONSE in char.properties[PROP_PERMISSIONS] ) From ff12fb69d8669d2f7e901f93161e1c5808147699 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:32:46 +0100 Subject: [PATCH 12/21] Type the write response helpers with the HAP JSON value types --- pyhap/accessory_driver.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index bef8e269..04f57d7b 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -87,13 +87,17 @@ MAX_PREPARED_WRITES_PER_CLIENT = 16 MAX_PREPARED_WRITE_TTL_SECONDS = 30.0 +# A characteristic value as HAP JSON carries it. +HAPValue = bool | int | float | str | None -def _stored_value(char: Any, value: Any) -> Any: + +def _stored_value(char: Characteristic, value: HAPValue) -> HAPValue: """Return the value ``client_update_value`` stores for ``value``.""" try: - return char.to_valid_value(value) + stored: HAPValue = char.to_valid_value(value) except ValueError: return value + return stored def _wrap_char_setter(char: Any, value: Any, client_addr: Any) -> tuple[int, Any]: @@ -347,7 +351,9 @@ def __init__( self.http_server = HAPServer(network_tuple, self) self.prepared_writes = {} # Write responses of readable characteristics, per client. - self._write_responses: dict[tuple[str, int], dict[Characteristic, Any]] = {} + self._write_responses: dict[ + tuple[str, int], dict[Characteristic, HAPValue] + ] = {} def listen_address(self) -> str | list[str]: """Return the address, or list of addresses, the HAP server listens on.""" @@ -1197,9 +1203,9 @@ def set_characteristics( def _store_write_response( self, char: Characteristic, - response: Any, + response: HAPValue, client_addr: tuple[str, int] | None, - ) -> Any: + ) -> HAPValue: """Keep the write response and return what to send. The written value of a write response characteristic, such as an SDP @@ -1213,7 +1219,7 @@ def _store_write_response( """ readable = HAP_PERMISSION_READ in char.properties[PROP_PERMISSIONS] try: - response = char.to_valid_value(response) + valid: HAPValue = char.to_valid_value(response) except ValueError: logger.debug( "%s: not keeping the write response, it is invalid: %s", @@ -1223,10 +1229,10 @@ def _store_write_response( return response if readable: if client_addr is not None: - self._write_responses.setdefault(client_addr, {})[char] = response + self._write_responses.setdefault(client_addr, {})[char] = valid else: - char.value = response - return response + char.value = valid + return valid def prepare( self, prepare_query: dict[str, Any], client_addr: tuple[str, int] From c9ed715b687bea4ed9e1f527f1098bdf739603d4 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:32:51 +0100 Subject: [PATCH 13/21] Explain why a private write puts the former value back --- pyhap/accessory_driver.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index 04f57d7b..387ba419 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -1141,7 +1141,11 @@ def set_characteristics( set_result, set_result_value = _wrap_char_setter( char, value, client_addr ) - # Put back the former value unless the setter set its own. + # A write with ``r`` to a readable characteristic is a request, + # e.g. a Setup Endpoints offer with the writer's SRTP keys. Only + # the writer reads the answer back, see _store_write_response. + # client_update_value stored the request as the value, so put + # back the former one unless the setter set a value of its own. if private_write and char.value == _stored_value(char, value): char.value = previous_value From 2e0018828bdfef72cbcf18621566e61b54de2af3 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 14:32:57 +0100 Subject: [PATCH 14/21] Don't tell subscribers the request of a write with r client_update_value takes notify=False for a write response request on a readable characteristic, so ev subscribers don't get a request that only the writer may read, e.g. a WiFi configuration with a key. HAPValue moves to pyhap.characteristic next to the formats. --- pyhap/accessory_driver.py | 20 ++++++++++++-------- pyhap/characteristic.py | 15 ++++++++++++--- tests/test_accessory_driver.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 11 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index 387ba419..08e3e817 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -36,7 +36,12 @@ from pyhap import util from pyhap.accessory import Accessory, Bridge, get_topic -from pyhap.characteristic import PROP_PERMISSIONS, Characteristic, CharacteristicError +from pyhap.characteristic import ( + PROP_PERMISSIONS, + Characteristic, + CharacteristicError, + HAPValue, +) from pyhap.const import ( HAP_PERMISSION_NOTIFY, HAP_PERMISSION_READ, @@ -87,9 +92,6 @@ MAX_PREPARED_WRITES_PER_CLIENT = 16 MAX_PREPARED_WRITE_TTL_SECONDS = 30.0 -# A characteristic value as HAP JSON carries it. -HAPValue = bool | int | float | str | None - def _stored_value(char: Characteristic, value: HAPValue) -> HAPValue: """Return the value ``client_update_value`` stores for ``value``.""" @@ -100,10 +102,12 @@ def _stored_value(char: Characteristic, value: HAPValue) -> HAPValue: return stored -def _wrap_char_setter(char: Any, value: Any, client_addr: Any) -> tuple[int, Any]: +def _wrap_char_setter( + char: Any, value: Any, client_addr: Any, notify: bool = True +) -> tuple[int, Any]: """Process an characteristic setter callback trapping and logging all exceptions.""" try: - response = char.client_update_value(value, client_addr) + response = char.client_update_value(value, client_addr, notify=notify) except Exception: logger.exception( "%s: Error while setting characteristic %s to %s", @@ -1124,7 +1128,7 @@ def set_characteristics( ) # A new write replaces a former response of this connection. The # request of a readable write response characteristic is not - # shared, whatever the setter does, except to ev subscribers. + # shared, whatever the setter does, also not with ev subscribers. if value is not None: self._write_responses.get(client_addr, {}).pop(char, None) private_write = ( @@ -1139,7 +1143,7 @@ def set_characteristics( if value is not None: set_result, set_result_value = _wrap_char_setter( - char, value, client_addr + char, value, client_addr, notify=not private_write ) # A write with ``r`` to a readable characteristic is a request, # e.g. a Setup Endpoints offer with the writer's SRTP keys. Only diff --git a/pyhap/characteristic.py b/pyhap/characteristic.py index a0b09a69..773aa57f 100644 --- a/pyhap/characteristic.py +++ b/pyhap/characteristic.py @@ -47,6 +47,9 @@ def _setter_wants_client_addr(setter: Callable[..., Any]) -> bool: # ### HAP Format ### +# A characteristic value as HAP JSON carries it. +HAPValue = bool | int | float | str | None + HAP_FORMAT_BOOL = "bool" HAP_FORMAT_INT = "int" HAP_FORMAT_FLOAT = "float" @@ -394,11 +397,17 @@ def set_value(self, value: Any, should_notify: bool = True) -> None: self.value = None def client_update_value( - self, value: Any, sender_client_addr: tuple[str, int] | None = None + self, + value: Any, + sender_client_addr: tuple[str, int] | None = None, + *, + notify: bool = True, ) -> Any: """Called from broker for value change in Home app. - Change self.value to value and call callback. + Change self.value to value and call callback. With ``notify`` False, + subscribers are not told about the written value, e.g. for a request + that only the writing connection may read back. """ original_value = value if not self._always_null or original_value is not None: @@ -421,7 +430,7 @@ def client_update_value( else: response = self.setter_callback(value) changed = self._value != previous_value - if changed: + if changed and notify: self.notify(sender_client_addr) if self._always_null: self.value = None diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index f7442732..13a5d471 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -880,6 +880,34 @@ def _setter(value: int) -> int: assert _read(driver, char_id, OTHER_CONTROLLER) == 42 +@pytest.mark.parametrize( + ("write_response", "notified"), + [ + pytest.param(True, [], id="private"), + pytest.param(False, [5], id="plain"), + ], +) +def test_private_write_not_notified( + driver: AccessoryDriver, write_response: bool, notified: list[int] +) -> None: + """Subscribers are not told the request of a write with ``r``.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "ev", "wr"], False) + char.getter_callback = None + char.setter_callback = MagicMock(return_value=7) + published = [] + with patch.object( + acc, "publish", side_effect=lambda value, *_: published.append(value) + ): + _write( + driver, + acc, + char, + **{HAP_REPR_VALUE: 5, HAP_REPR_WRITE_RESPONSE: write_response}, + ) + + assert published == notified + + @pytest.mark.parametrize("reader", [WRITER, OTHER_CONTROLLER], ids=["writer", "other"]) @pytest.mark.parametrize( "make_setter", From d130cfbc3a913d89fc64f0167602eb0715cb56fb Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 14:33:52 +0100 Subject: [PATCH 15/21] Type the timed write pid and ignore a pid that is not an int A pid of another type, e.g. a list, raised a TypeError instead of answering -70410. --- pyhap/accessory_driver.py | 5 ++++- tests/test_timed_write.py | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index e33f39a5..8651c9a4 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -1247,8 +1247,11 @@ 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: + def _take_prepared_write(self, pid: int, client_addr: tuple[str, int]) -> bool: """Consume the prepared write ``pid`` and return whether it is still valid.""" + if not isinstance(pid, int): + # 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 diff --git a/tests/test_timed_write.py b/tests/test_timed_write.py index cac75f8b..700a4d37 100644 --- a/tests/test_timed_write.py +++ b/tests/test_timed_write.py @@ -100,13 +100,13 @@ def test_prepared_write_is_applied( @pytest.mark.parametrize( ("write_pid", "delay"), - [(8, 0.0), (7, 2.6)], - ids=["unknown_pid", "expired_pid"], + [(8, 0.0), (7, 2.6), ([7], 0.0)], + ids=["unknown_pid", "expired_pid", "unhashable_pid"], ) def test_unprepared_write_is_ignored( driver: AccessoryDriver, door: WritableChar, - write_pid: int, + write_pid: Any, delay: float, ) -> None: """Test a write with an unknown or expired pid is ignored as a whole.""" From 3a593d75a59d4570abcfea0fd58b3440b83441b6 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 14:34:46 +0100 Subject: [PATCH 16/21] Name the fields of a pending async write and keep write_response a bool A write asks for a response only with "r": true. --- pyhap/accessory_driver.py | 41 +++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index 538cfe56..00c4d40d 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -31,7 +31,7 @@ import tempfile import threading import time -from typing import Any +from typing import Any, NamedTuple from zeroconf import ServiceInfo from zeroconf.asyncio import AsyncZeroconf @@ -172,9 +172,13 @@ def _wrap_service_setter(service: Any, chars: Any, client_addr: Any) -> Any: return HAP_SERVER_STATUS.SUCCESS -# A characteristic write whose setter is a coroutine: the result entry to -# complete, the coroutine, whether a write response was asked for and the char. -_PendingWrite = tuple[dict[str, Any], Coroutine[Any, Any, Any], bool, Characteristic] +class _PendingWrite(NamedTuple): + """A characteristic write whose setter returned a coroutine.""" + + result: dict[str, Any] # The result entry to complete. + coro: Coroutine[Any, Any, Any] + write_response: bool # Whether the controller asked for a write response. + char: Characteristic def _set_characteristics_response( @@ -1256,7 +1260,7 @@ def _run_characteristic_writes( # Only characteristics with the write response permission answer # a write with a value, and only when the controller asks for it. write_response = ( - query.get(HAP_REPR_WRITE_RESPONSE, False) + query.get(HAP_REPR_WRITE_RESPONSE) is True and HAP_PERMISSION_WRITE_RESPONSE in char.properties[PROP_PERMISSIONS] ) # A new write replaces a former response of this connection. The @@ -1288,7 +1292,9 @@ def _run_characteristic_writes( result: dict[str, Any] = {HAP_REPR_STATUS: set_result} if inspect.iscoroutine(set_result_value): - pending.append((result, set_result_value, write_response, char)) + pending.append( + _PendingWrite(result, set_result_value, write_response, char) + ) elif set_result_value is not None and write_response: result[HAP_REPR_VALUE] = self._store_write_response( char, set_result_value, client_addr @@ -1334,10 +1340,7 @@ async def _async_finish_set_characteristics( timeout: float, ) -> dict[str, Any] | None: """Wait for the coroutine setters, then build the response.""" - tasks = { - asyncio.ensure_future(coro): (result, write_response, char) - for result, coro, write_response, char in pending - } + tasks = {asyncio.ensure_future(write.coro): write for write in pending} try: done, not_done = await asyncio.wait(tasks, timeout=timeout) except asyncio.CancelledError: @@ -1346,32 +1349,32 @@ async def _async_finish_set_characteristics( raise for task in not_done: task.cancel() - result, _, char = tasks[task] + write = tasks[task] logger.warning( "%s: Timed out setting characteristic %s", client_addr, - char.display_name, + write.char.display_name, ) - result[HAP_REPR_STATUS] = HAP_SERVER_STATUS.OPERATION_TIMED_OUT + write.result[HAP_REPR_STATUS] = HAP_SERVER_STATUS.OPERATION_TIMED_OUT for task in done: - result, write_response, char = tasks[task] + write = tasks[task] try: outcome = task.result() except (Exception, asyncio.CancelledError): logger.exception( "%s: Error while setting characteristic %s", client_addr, - char.display_name, + write.char.display_name, ) - result[HAP_REPR_STATUS] = ( + write.result[HAP_REPR_STATUS] = ( HAP_SERVER_STATUS.SERVICE_COMMUNICATION_FAILURE ) continue - if outcome is not None and write_response: + if outcome is not None and write.write_response: # A response queued when the connection closes is dropped by # the deferred cleanup in connection_lost. - result[HAP_REPR_VALUE] = self._store_write_response( - char, outcome, client_addr + write.result[HAP_REPR_VALUE] = self._store_write_response( + write.char, outcome, client_addr ) return _set_characteristics_response(results) From 770db1795e0bc76e20f63aeac09d1ba3d3ad0cde Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 14:50:39 +0100 Subject: [PATCH 17/21] Run test_start_from_sync on the driver's own loop It set a new default executor on the shared test loop and shut it down when the driver stopped, so later tests on the same xdist worker failed with "cannot schedule new futures after shutdown". The driver now makes its own loop and executor, as a sync start does. --- tests/test_accessory_driver.py | 36 +++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index 8443632b..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") From 06885188ec8b3ae92b11404ac19f65e36d475867 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 16:55:17 +0100 Subject: [PATCH 18/21] Refuse a prepare whose ttl or pid is not an integer The write only accepts an integer pid, so a prepare with another pid could never be used. A ttl too large for a float is refused too, and an entry expires at the same moment for prepare and write. Tests cover a rejected timed write with several characteristics and an explicit null value on a tw characteristic. --- pyhap/accessory_driver.py | 25 +++++++----- tests/test_timed_write.py | 81 ++++++++++++++++++++++++++++----------- 2 files changed, 74 insertions(+), 32 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index 8651c9a4..97b2d7be 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -94,6 +94,11 @@ MAX_PREPARED_WRITE_TTL_SECONDS = 30.0 +def _is_hap_int(value: Any) -> bool: + """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: @@ -306,11 +311,9 @@ def __init__( :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. When False, such a write is - applied and only logged at debug level: a Home hub has been seen - writing HomeKit Camera Active without a timed write, and the HomeKit - Secure Video guide gives Streaming Enabled and Motion Enabled tw too, - so enforcing it would break HKSV with such hubs. + 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: @@ -1247,10 +1250,10 @@ def set_characteristics( return {HAP_REPR_CHARS: characteristics} if nonempty_results_exist else None - def _take_prepared_write(self, pid: int, client_addr: tuple[str, int]) -> bool: + 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 isinstance(pid, int): - # Controller input; a list or dict is not even hashable. + 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 @@ -1308,6 +1311,8 @@ def prepare( try: ttl = prepare_query[HAP_REPR_TTL] pid = prepare_query[HAP_REPR_PID] + if not (_is_hap_int(ttl) and _is_hap_int(pid)): + raise TypeError("ttl and pid must be integers") now = time.monotonic() pending = self.prepared_writes.setdefault(client_addr, {}) # Prune expired entries and cap the number of pending writes so a @@ -1316,13 +1321,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 (KeyError, TypeError, ValueError, OverflowError): return {HAP_REPR_STATUS: HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST} return {HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS} diff --git a/tests/test_timed_write.py b/tests/test_timed_write.py index 700a4d37..0ed09d65 100644 --- a/tests/test_timed_write.py +++ b/tests/test_timed_write.py @@ -24,6 +24,7 @@ CLIENT = ("1.2.3.4", 5000) NOW = 1000.0 +REJECTED = HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST class WritableChar(NamedTuple): @@ -130,6 +131,46 @@ def test_unprepared_write_is_ignored( assert get_topic(door.aid, door.iid) not in driver.topics +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: 10**400, HAP_REPR_PID: 7}, + ], + ids=["str_pid", "float_pid", "bool_pid", "str_ttl", "overflowing_ttl"], +) +def test_prepare_refuses_non_integers( + driver: AccessoryDriver, prepare_query: dict[str, Any] +) -> None: + """Test a prepare whose ttl or pid is not an integer 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, @@ -165,15 +206,9 @@ def test_prepared_pid_belongs_to_its_connection( door.setter.assert_not_called() -def _rejection(char: WritableChar, status: int | None) -> dict[str, Any] | None: - """Return the expected write response, None for a plain success.""" - if status is None: - return None - return { - HAP_REPR_CHARS: [ - {HAP_REPR_AID: char.aid, HAP_REPR_IID: char.iid, HAP_REPR_STATUS: status} - ] - } +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: @@ -181,24 +216,25 @@ def _highest_level(records: list[logging.LogRecord]) -> int: return max((record.levelno for record in records), default=0) -REJECTED = HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST - - @pytest.mark.parametrize( - ("enforce", "name", "request_extra", "status", "calls", "logged"), + ("enforce", "name", "request_extra", "value", "statuses", "calls", "logged"), [ - (True, "HomeKitCameraActive", {}, REJECTED, 0, 0), - (True, "HomeKitCameraActive", {HAP_REPR_PID: 7}, None, 1, 0), - (True, "TargetDoorState", {}, None, 1, 0), - (False, "HomeKitCameraActive", {}, None, 1, 1), - (False, "HomeKitCameraActive", {HAP_REPR_PID: 7}, None, 1, 0), - (False, "TargetDoorState", {}, None, 1, 0), + (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", ], @@ -210,7 +246,8 @@ def test_timed_write_permission( enforce: bool, name: str, request_extra: dict[str, Any], - status: int | None, + value: int | None, + statuses: list[int], calls: int, logged: int, ) -> None: @@ -221,14 +258,14 @@ def test_timed_write_permission( 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: 0} + {HAP_REPR_AID: char.aid, HAP_REPR_IID: char.iid, HAP_REPR_VALUE: value} ], **request_extra, } response = driver.set_characteristics(query, CLIENT) - assert response == _rejection(char, status) + 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. From 71119dc0ed8ebd280c0f35dbf9a08bf4f0fc4e6b Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 16:59:52 +0100 Subject: [PATCH 19/21] Check the prepare ttl and pid before the try and test the edges A bool pid, a float ttl and the last moment of a TTL are covered now. --- pyhap/accessory_driver.py | 16 ++++++++------- tests/test_timed_write.py | 42 +++++++++++++++++++++++++++++++++------ 2 files changed, 45 insertions(+), 13 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index 97b2d7be..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 @@ -94,7 +94,7 @@ MAX_PREPARED_WRITE_TTL_SECONDS = 30.0 -def _is_hap_int(value: Any) -> bool: +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) @@ -1308,11 +1308,13 @@ 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] - if not (_is_hap_int(ttl) and _is_hap_int(pid)): - raise TypeError("ttl and pid must be integers") now = time.monotonic() pending = self.prepared_writes.setdefault(client_addr, {}) # Prune expired entries and cap the number of pending writes so a @@ -1327,7 +1329,7 @@ def prepare( 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, OverflowError): + except OverflowError: return {HAP_REPR_STATUS: HAP_SERVER_STATUS.INVALID_VALUE_IN_REQUEST} return {HAP_REPR_STATUS: HAP_SERVER_STATUS.SUCCESS} diff --git a/tests/test_timed_write.py b/tests/test_timed_write.py index 0ed09d65..929c919a 100644 --- a/tests/test_timed_write.py +++ b/tests/test_timed_write.py @@ -101,8 +101,8 @@ def test_prepared_write_is_applied( @pytest.mark.parametrize( ("write_pid", "delay"), - [(8, 0.0), (7, 2.6), ([7], 0.0)], - ids=["unknown_pid", "expired_pid", "unhashable_pid"], + [(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, @@ -112,7 +112,7 @@ def test_unprepared_write_is_ignored( ) -> 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: 7}, CLIENT) + 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 @@ -131,6 +131,20 @@ def test_unprepared_write_is_ignored( 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], @@ -159,14 +173,30 @@ def test_unprepared_write_ignores_every_characteristic( {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", ], - ids=["str_pid", "float_pid", "bool_pid", "str_ttl", "overflowing_ttl"], ) def test_prepare_refuses_non_integers( - driver: AccessoryDriver, prepare_query: dict[str, Any] + driver: AccessoryDriver, prepare_query: Any ) -> None: - """Test a prepare whose ttl or pid is not an integer is refused.""" + """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, {}) == {} From fe8c4ed539a3853851e279faeb2af5a8a3a64742 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:15:48 +0100 Subject: [PATCH 20/21] Tell subscribers about an async write once its setter succeeded A coroutine setter that fails or times out no longer sends an event for the value it did not set, the same as a sync setter that raises. A setter that runs on after its cancellation has its error retrieved and logged at debug level. Any truthy "r" asks for a write response again. --- pyhap/accessory_driver.py | 52 ++++++++++++--- pyhap/characteristic.py | 19 ++++-- pyhap/hap_handler.py | 2 +- tests/test_accessory_driver.py | 111 +++++++++++++++++++++++++++++++++ 4 files changed, 169 insertions(+), 15 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index 6c63b876..e9caf782 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -184,6 +184,14 @@ class _PendingWrite(NamedTuple): coro: Coroutine[Any, Any, Any] write_response: bool # Whether the controller asked for a write response. char: Characteristic + previous_value: Any # The value before the write, to tell if it changed. + notify: bool # Whether subscribers learn the value once the setter is done. + + +def _retrieve_late_result(task: asyncio.Future[Any]) -> None: + """Retrieve the outcome of a setter that ran on after it was cancelled.""" + if not task.cancelled() and (err := task.exception()) is not None: + logger.debug("Setter failed after it was cancelled", exc_info=err) def _set_characteristics_response( @@ -1109,9 +1117,12 @@ def set_characteristics( A characteristic setter may be a coroutine function. Writes from a controller wait for it before they are answered. Called directly, this - method only schedules the coroutine and does not wait for it. - Accessory and service setters must not be coroutine functions; a - coroutine they return is closed with a warning. + method only schedules the coroutine and does not wait for it. Either + way subscribers learn the value once the coroutine succeeded, and it + should use its argument, not the characteristic's value, which a write + response request has already put back. Accessory and service setters + must not be coroutine functions. A coroutine they return is closed + with a warning. :param chars_query: A configuration query. For example: @@ -1130,10 +1141,19 @@ def set_characteristics( :type chars_query: dict """ results, pending = self._apply_characteristic_writes(chars_query, client_addr) - for _, coro, _, _ in pending: - self.add_job(coro) + for write in pending: + self.add_job(self._finish_pending_write(write, client_addr)) return _set_characteristics_response(results) + async def _finish_pending_write( + self, write: _PendingWrite, client_addr: tuple[str, int] + ) -> None: + """Run a coroutine setter of a direct call, then tell subscribers.""" + await write.coro + write.char._client_update_finished( # noqa: SLF001 + write.previous_value, client_addr, write.notify + ) + def _set_characteristics_for_controller( self, chars_query: dict[str, Any], @@ -1145,7 +1165,7 @@ def _set_characteristics_for_controller( Returns an awaitable of the response if a characteristic setter is a coroutine function. Accessory and service setters run before those coroutines start. A setter still running after ``timeout`` seconds is - cancelled and its characteristic answers -70408; cancelling the + cancelled and its characteristic answers -70408. Cancelling the awaitable, e.g. because the connection closed, cancels the setters. """ results, pending = self._apply_characteristic_writes(chars_query, client_addr) @@ -1165,8 +1185,8 @@ def _apply_characteristic_writes( except BaseException: # A malformed later entry must not leave a setter coroutine # un-awaited. - for _, coro, _, _ in pending: - coro.close() + for write in pending: + write.coro.close() raise def _run_characteristic_writes( @@ -1263,7 +1283,7 @@ def _run_characteristic_writes( # Only characteristics with the write response permission answer # a write with a value, and only when the controller asks for it. write_response = ( - query.get(HAP_REPR_WRITE_RESPONSE) is True + bool(query.get(HAP_REPR_WRITE_RESPONSE)) and HAP_PERMISSION_WRITE_RESPONSE in char.properties[PROP_PERMISSIONS] ) # A new write replaces a former response of this connection. The @@ -1296,7 +1316,14 @@ def _run_characteristic_writes( result: dict[str, Any] = {HAP_REPR_STATUS: set_result} if inspect.iscoroutine(set_result_value): pending.append( - _PendingWrite(result, set_result_value, write_response, char) + _PendingWrite( + result, + set_result_value, + write_response, + char, + previous_value, + not private_write, + ) ) elif set_result_value is not None and write_response: result[HAP_REPR_VALUE] = self._store_write_response( @@ -1349,9 +1376,11 @@ async def _async_finish_set_characteristics( except asyncio.CancelledError: for task in tasks: task.cancel() + task.add_done_callback(_retrieve_late_result) raise for task in not_done: task.cancel() + task.add_done_callback(_retrieve_late_result) write = tasks[task] logger.warning( "%s: Timed out setting characteristic %s", @@ -1373,6 +1402,9 @@ async def _async_finish_set_characteristics( HAP_SERVER_STATUS.SERVICE_COMMUNICATION_FAILURE ) continue + write.char._client_update_finished( # noqa: SLF001 + write.previous_value, client_addr, write.notify + ) if outcome is not None and write.write_response: # A response queued when the connection closes is dropped by # the deferred cleanup in connection_lost. diff --git a/pyhap/characteristic.py b/pyhap/characteristic.py index c22fdbc7..0172f4de 100644 --- a/pyhap/characteristic.py +++ b/pyhap/characteristic.py @@ -407,7 +407,9 @@ def client_update_value( """Called from broker for value change in Home app. Change self.value to value and call callback. Returns what the callback - returned, which the caller has to await if it is awaitable. With + returned. A coroutine the callback returns is for the caller to await, + then the caller finishes the update with ``_client_update_finished``, + so subscribers only learn the value once the setter succeeded. With ``notify`` False, subscribers are not told about the written value, e.g. for a request that only the writing connection may read back. """ @@ -431,12 +433,21 @@ def client_update_value( response = self.setter_callback(value, sender_client_addr) else: response = self.setter_callback(value) - changed = self._value != previous_value - if changed and notify: + if not inspect.iscoroutine(response): + self._client_update_finished(previous_value, sender_client_addr, notify) + return response + + def _client_update_finished( + self, + previous_value: Any, + sender_client_addr: tuple[str, int] | None, + notify: bool, + ) -> None: + """Tell subscribers about a client's write once its setter is done.""" + if notify and self._value != previous_value: self.notify(sender_client_addr) if self._always_null: self.value = None - return response def _setter_wants_addr(self) -> bool: """Whether the current setter opts into the sender's client address. diff --git a/pyhap/hap_handler.py b/pyhap/hap_handler.py index d23f2f56..8c7844cd 100644 --- a/pyhap/hap_handler.py +++ b/pyhap/hap_handler.py @@ -719,7 +719,7 @@ def handle_set_characteristics(self) -> None: ) assert self.response is not None # noqa: S101 if inspect.isawaitable(result): - # A setter is still running; answer once it is done. The driver + # A setter is still running, answer once it is done. The driver # times out slow setters itself. self.response.task = asyncio.ensure_future( self._async_set_characteristics_body(self.response, result) diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index 7f8637fd..1c419171 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -3,6 +3,7 @@ import asyncio from functools import partial import inspect +import logging import os import tempfile import time @@ -1218,6 +1219,116 @@ async def test_public_set_characteristics_schedules_async_setters( await asyncio.wait_for(setter.ran.wait(), ASYNC_TIMEOUT) +class _NotifyCheckingSetter: + """An async setter that records how often the char notified when it ran.""" + + def __init__(self, char: Characteristic, fail: bool) -> None: + self.char = char + self.fail = fail + self.notified_before: list[int] = [] + + async def __call__(self, value: int) -> None: + await asyncio.sleep(0) + self.notified_before.append(self.char.notify.call_count) + if self.fail: + raise ValueError("setter failed") + + +@pytest.mark.parametrize( + ("fail", "notified"), + [pytest.param(False, 1, id="success"), pytest.param(True, 0, id="error")], +) +async def test_async_setter_notifies_once_it_succeeded( + driver: AccessoryDriver, fail: bool, notified: int +) -> None: + """Subscribers learn a written value only after the async setter succeeded.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "ev"], False) + char.value = 5 + setter = _NotifyCheckingSetter(char, fail) + char.setter_callback = setter + + with patch.object(Characteristic, "notify") as notify: + await _controller_write(driver, acc, char) + + assert setter.notified_before == [0] + assert notify.call_count == notified + + +async def test_public_write_notifies_once_the_async_setter_ran( + driver: AccessoryDriver, +) -> None: + """Called directly, subscribers also learn the value after the setter ran.""" + driver.loop = asyncio.get_running_loop() + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "ev"], False) + char.value = 5 + setter = _RecordingSetter() + char.setter_callback = setter + notified = asyncio.Event() + + with patch.object( + Characteristic, "notify", side_effect=lambda *_: notified.set() + ) as notify: + assert _write(driver, acc, char) is None + assert notify.call_count == 0 + await asyncio.wait_for(notified.wait(), ASYNC_TIMEOUT) + + assert setter.ran.is_set() + notify.assert_called_once_with(WRITER) + + +class _StubbornSetter: + """An async setter that ignores its cancellation and fails afterwards.""" + + def __init__(self) -> None: + self.failed = asyncio.Event() + + async def __call__(self, value: int) -> None: + try: + await asyncio.sleep(10) + except asyncio.CancelledError: + await asyncio.sleep(0) + self.failed.set() + raise ValueError("failed after the cancellation") from None + + +async def test_setter_failing_after_its_timeout_is_retrieved( + driver: AccessoryDriver, caplog: pytest.LogCaptureFixture +) -> None: + """A setter that runs on after its timeout has its error logged, not lost.""" + caplog.set_level(logging.DEBUG, "pyhap.accessory_driver") + acc, char, _ = _add_bridged_or_standalone(driver, ["pw"], False) + setter = _StubbornSetter() + char.setter_callback = setter + query = { + HAP_REPR_CHARS: [ + { + HAP_REPR_AID: acc.aid, + HAP_REPR_IID: acc.iid_manager.get_iid(char), + HAP_REPR_VALUE: 0, + } + ] + } + + result = driver._set_characteristics_for_controller(query, WRITER, 0.05) + assert inspect.isawaitable(result) + await result + await asyncio.wait_for(setter.failed.wait(), ASYNC_TIMEOUT) + await asyncio.sleep(0) + + assert "Setter failed after it was cancelled" in caplog.text + + +@pytest.mark.parametrize("flag", [True, 1], ids=["true", "one"]) +def test_write_response_flag_is_truthy(driver: AccessoryDriver, flag: Any) -> None: + """Any truthy "r" asks for a write response, as before.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pw", "wr"], False) + char.setter_callback = MagicMock(return_value=1) + + response = _write(driver, acc, char, **{HAP_REPR_WRITE_RESPONSE: flag}) + + assert response[HAP_REPR_CHARS][0][HAP_REPR_VALUE] == 1 + + async def test_setter_returning_a_task_is_not_awaited( driver: AccessoryDriver, ) -> None: From b1caf00caabcec3aa5b40517c27d2c0c443792ca Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:23:39 +0100 Subject: [PATCH 21/21] Keep client_update_value notifying at once unless the caller defers it The driver asks for the deferred notify with defer_notify and finishes the update with the public client_update_finished. A failing coroutine setter of a direct set_characteristics call is logged with its name, and setters done before a write is cancelled still tell subscribers. --- pyhap/accessory_driver.py | 47 +++++- pyhap/characteristic.py | 29 ++-- tests/test_accessory_driver.py | 269 +++++++++++++++++++++++++++++---- 3 files changed, 298 insertions(+), 47 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index e9caf782..7f7326f2 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -115,7 +115,10 @@ def _wrap_char_setter( ) -> tuple[int, Any]: """Process an characteristic setter callback trapping and logging all exceptions.""" try: - response = char.client_update_value(value, client_addr, notify=notify) + # The driver finishes the update of a coroutine setter once it is done. + response = char.client_update_value( + value, client_addr, notify=notify, defer_notify=True + ) except Exception: logger.exception( "%s: Error while setting characteristic %s to %s", @@ -1149,9 +1152,17 @@ async def _finish_pending_write( self, write: _PendingWrite, client_addr: tuple[str, int] ) -> None: """Run a coroutine setter of a direct call, then tell subscribers.""" - await write.coro - write.char._client_update_finished( # noqa: SLF001 - write.previous_value, client_addr, write.notify + try: + await write.coro + except Exception: + logger.exception( + "%s: Error while setting characteristic %s", + client_addr, + write.char.display_name, + ) + return + write.char.client_update_finished( + write.previous_value, client_addr, notify=write.notify ) def _set_characteristics_for_controller( @@ -1374,7 +1385,10 @@ async def _async_finish_set_characteristics( try: done, not_done = await asyncio.wait(tasks, timeout=timeout) except asyncio.CancelledError: - for task in tasks: + for task, write in tasks.items(): + if task.done(): + self._finish_done_write(task, write, client_addr) + continue task.cancel() task.add_done_callback(_retrieve_late_result) raise @@ -1402,8 +1416,8 @@ async def _async_finish_set_characteristics( HAP_SERVER_STATUS.SERVICE_COMMUNICATION_FAILURE ) continue - write.char._client_update_finished( # noqa: SLF001 - write.previous_value, client_addr, write.notify + write.char.client_update_finished( + write.previous_value, client_addr, notify=write.notify ) if outcome is not None and write.write_response: # A response queued when the connection closes is dropped by @@ -1413,6 +1427,25 @@ async def _async_finish_set_characteristics( ) return _set_characteristics_response(results) + @staticmethod + def _finish_done_write( + task: asyncio.Future[Any], write: _PendingWrite, client_addr: tuple[str, int] + ) -> None: + """Tell subscribers about a setter done before the write was cancelled.""" + if task.cancelled(): + return + if (err := task.exception()) is not None: + logger.error( + "%s: Error while setting characteristic %s", + client_addr, + write.char.display_name, + exc_info=err, + ) + return + write.char.client_update_finished( + write.previous_value, client_addr, notify=write.notify + ) + 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): diff --git a/pyhap/characteristic.py b/pyhap/characteristic.py index 0172f4de..3dd44c9f 100644 --- a/pyhap/characteristic.py +++ b/pyhap/characteristic.py @@ -403,15 +403,17 @@ def client_update_value( sender_client_addr: tuple[str, int] | None = None, *, notify: bool = True, + defer_notify: bool = False, ) -> Any: """Called from broker for value change in Home app. Change self.value to value and call callback. Returns what the callback - returned. A coroutine the callback returns is for the caller to await, - then the caller finishes the update with ``_client_update_finished``, - so subscribers only learn the value once the setter succeeded. With - ``notify`` False, subscribers are not told about the written value, - e.g. for a request that only the writing connection may read back. + returned. With ``notify`` False, subscribers are not told about the + written value, e.g. for a request that only the writing connection may + read back. With ``defer_notify``, a coroutine the callback returns is + for the caller to await, and the caller then calls + :meth:`client_update_finished`, so subscribers only learn the value + once the setter succeeded. """ original_value = value if not self._always_null or original_value is not None: @@ -433,17 +435,24 @@ def client_update_value( response = self.setter_callback(value, sender_client_addr) else: response = self.setter_callback(value) - if not inspect.iscoroutine(response): - self._client_update_finished(previous_value, sender_client_addr, notify) + if not (defer_notify and inspect.iscoroutine(response)): + self.client_update_finished( + previous_value, sender_client_addr, notify=notify + ) return response - def _client_update_finished( + def client_update_finished( self, previous_value: Any, sender_client_addr: tuple[str, int] | None, - notify: bool, + *, + notify: bool = True, ) -> None: - """Tell subscribers about a client's write once its setter is done.""" + """Finish a client's write, e.g. after awaiting its coroutine setter. + + Tells subscribers about the new value if it differs from + ``previous_value``, see :meth:`client_update_value`. + """ if notify and self._value != previous_value: self.notify(sender_client_addr) if self._always_null: diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index 1c419171..77d40b1c 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -3,19 +3,18 @@ import asyncio from functools import partial import inspect -import logging import os import tempfile import time from typing import Any, NoReturn -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import ANY, AsyncMock, MagicMock, call, patch from uuid import uuid1 from cryptography.hazmat.primitives import serialization import pytest from zeroconf import InterfaceChoice -from pyhap import util +from pyhap import accessory_driver, util from pyhap.accessory import STANDALONE_AID, Accessory, Bridge from pyhap.accessory_driver import ( MAX_FAILED_PAIRING_ATTEMPTS, @@ -1220,38 +1219,84 @@ async def test_public_set_characteristics_schedules_async_setters( class _NotifyCheckingSetter: - """An async setter that records how often the char notified when it ran.""" + """An async setter that records how often subscribers were told when it ran.""" - def __init__(self, char: Characteristic, fail: bool) -> None: - self.char = char + def __init__(self, notify: MagicMock, fail: bool) -> None: + self.notify = notify self.fail = fail self.notified_before: list[int] = [] async def __call__(self, value: int) -> None: await asyncio.sleep(0) - self.notified_before.append(self.char.notify.call_count) + self.notified_before.append(self.notify.call_count) if self.fail: raise ValueError("setter failed") @pytest.mark.parametrize( - ("fail", "notified"), - [pytest.param(False, 1, id="success"), pytest.param(True, 0, id="error")], + ("fail", "calls"), + [ + pytest.param(False, [call(WRITER)], id="success"), + pytest.param(True, [], id="error"), + ], ) async def test_async_setter_notifies_once_it_succeeded( - driver: AccessoryDriver, fail: bool, notified: int + driver: AccessoryDriver, fail: bool, calls: list[Any] ) -> None: """Subscribers learn a written value only after the async setter succeeded.""" acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "ev"], False) char.value = 5 - setter = _NotifyCheckingSetter(char, fail) - char.setter_callback = setter with patch.object(Characteristic, "notify") as notify: + setter = _NotifyCheckingSetter(notify, fail) + char.setter_callback = setter await _controller_write(driver, acc, char) assert setter.notified_before == [0] - assert notify.call_count == notified + assert notify.call_args_list == calls + + +class _ValueSettingSetter: + """An async setter that stores a value of its own and answers 1.""" + + def __init__(self, char: Characteristic) -> None: + self.char = char + self.done = asyncio.Event() + + async def __call__(self, value: int) -> int: + await asyncio.sleep(0) + self.char.value = 7 + self.done.set() + return 1 + + +@pytest.mark.parametrize("own_value", [False, True], ids=["request", "own-value"]) +async def test_private_async_write_does_not_notify( + driver: AccessoryDriver, own_value: bool +) -> None: + """A write response request through an async setter tells no subscriber.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "ev", "wr"], False) + char.value = 5 + setters = {False: _async_return_one, True: _ValueSettingSetter(char)} + char.setter_callback = setters[own_value] + + with patch.object(Characteristic, "notify") as notify: + await _controller_write(driver, acc, char, **{HAP_REPR_WRITE_RESPONSE: True}) + + notify.assert_not_called() + + +def test_direct_client_update_notifies_at_once(driver: AccessoryDriver) -> None: + """Without defer_notify, client_update_value notifies as before.""" + _, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "ev"], False) + char.value = 5 + char.setter_callback = _async_return_none + + with patch.object(Characteristic, "notify") as notify: + coro = char.client_update_value(0, WRITER) + + coro.close() + notify.assert_called_once_with(WRITER) async def test_public_write_notifies_once_the_async_setter_ran( @@ -1264,41 +1309,205 @@ async def test_public_write_notifies_once_the_async_setter_ran( setter = _RecordingSetter() char.setter_callback = setter notified = asyncio.Event() + ran_when_notified: list[bool] = [] - with patch.object( - Characteristic, "notify", side_effect=lambda *_: notified.set() - ) as notify: + def _record(*_: Any) -> None: + ran_when_notified.append(setter.ran.is_set()) + notified.set() + + with patch.object(Characteristic, "notify", side_effect=_record) as notify: assert _write(driver, acc, char) is None - assert notify.call_count == 0 await asyncio.wait_for(notified.wait(), ASYNC_TIMEOUT) - assert setter.ran.is_set() + assert ran_when_notified == [True] notify.assert_called_once_with(WRITER) +async def test_public_write_logs_a_failing_async_setter( + driver: AccessoryDriver, +) -> None: + """Called directly, a failing async setter is logged with its name.""" + driver.loop = asyncio.get_running_loop() + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "ev"], False) + char.value = 5 + logged = asyncio.Event() + char.setter_callback = _async_raise + + with ( + patch.object(Characteristic, "notify") as notify, + patch.object( + accessory_driver.logger, + "exception", + side_effect=lambda *_: logged.set(), + ) as log_exception, + ): + assert _write(driver, acc, char) is None + await asyncio.wait_for(logged.wait(), ASYNC_TIMEOUT) + + log_exception.assert_called_once_with( + "%s: Error while setting characteristic %s", WRITER, char.display_name + ) + notify.assert_not_called() + + +@pytest.mark.parametrize( + ("fast_setter", "calls"), + [ + pytest.param(_async_return_none, [call(WRITER)], id="success"), + pytest.param(_async_raise, [], id="error"), + ], +) +async def test_cancelled_write_notifies_setters_that_finished( + driver: AccessoryDriver, fast_setter: Any, calls: list[Any] +) -> None: + """A setter done before the write is cancelled tells subscribers if it worked.""" + acc = Accessory(driver, "Test Accessory") + service = Service(uuid1(), "Test Service") + props = {PROP_FORMAT: HAP_FORMAT_INT, PROP_PERMISSIONS: ["pr", "pw", "ev"]} + fast = Characteristic("Fast", uuid1(), props) + fast.setter_callback = fast_setter + slow = Characteristic("Slow", uuid1(), props) + slow_setter = _SlowSetter() + slow.setter_callback = slow_setter + service.add_characteristic(fast, slow) + acc.add_service(service) + driver.add_accessory(acc) + fast.value = slow.value = 5 + fast_iid = acc.iid_manager.get_iid(fast) + slow_iid = acc.iid_manager.get_iid(slow) + query = { + HAP_REPR_CHARS: [ + {HAP_REPR_AID: acc.aid, HAP_REPR_IID: fast_iid, HAP_REPR_VALUE: 0}, + {HAP_REPR_AID: acc.aid, HAP_REPR_IID: slow_iid, HAP_REPR_VALUE: 0}, + ] + } + + with patch.object(Characteristic, "notify") as notify: + result = driver._set_characteristics_for_controller( + query, WRITER, ASYNC_TIMEOUT + ) + assert inspect.isawaitable(result) + task = asyncio.ensure_future(result) + await asyncio.wait_for(slow_setter.started.wait(), ASYNC_TIMEOUT) + await asyncio.sleep(0) + task.cancel() + await asyncio.gather(task, return_exceptions=True) + + assert notify.call_args_list == calls + assert task.cancelled() + + +async def test_cancelled_write_logs_a_setter_that_failed_before( + driver: AccessoryDriver, +) -> None: + """A setter that failed before the write is cancelled is logged once.""" + acc = Accessory(driver, "Test Accessory") + service = Service(uuid1(), "Test Service") + props = {PROP_FORMAT: HAP_FORMAT_INT, PROP_PERMISSIONS: ["pw"]} + failing = Characteristic("Failing", uuid1(), props) + failing.setter_callback = _async_raise + slow = Characteristic("Slow", uuid1(), props) + slow_setter = _SlowSetter() + slow.setter_callback = slow_setter + service.add_characteristic(failing, slow) + acc.add_service(service) + driver.add_accessory(acc) + failing_iid = acc.iid_manager.get_iid(failing) + slow_iid = acc.iid_manager.get_iid(slow) + query = { + HAP_REPR_CHARS: [ + {HAP_REPR_AID: acc.aid, HAP_REPR_IID: failing_iid, HAP_REPR_VALUE: 0}, + {HAP_REPR_AID: acc.aid, HAP_REPR_IID: slow_iid, HAP_REPR_VALUE: 0}, + ] + } + result = driver._set_characteristics_for_controller(query, WRITER, ASYNC_TIMEOUT) + assert inspect.isawaitable(result) + task = asyncio.ensure_future(result) + await asyncio.wait_for(slow_setter.started.wait(), ASYNC_TIMEOUT) + await asyncio.sleep(0) + + with ( + patch.object(accessory_driver.logger, "error") as log_error, + patch.object(accessory_driver.logger, "debug") as log_debug, + ): + task.cancel() + await asyncio.gather(task, return_exceptions=True) + await asyncio.wait_for(slow_setter.cancelled.wait(), ASYNC_TIMEOUT) + + log_error.assert_called_once_with( + "%s: Error while setting characteristic %s", + WRITER, + "Failing", + exc_info=ANY, + ) + log_debug.assert_not_called() + + +async def test_public_private_write_with_own_value_does_not_notify( + driver: AccessoryDriver, +) -> None: + """Called directly, a write response request tells no subscriber either.""" + driver.loop = asyncio.get_running_loop() + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw", "ev", "wr"], False) + char.value = 5 + setter = _ValueSettingSetter(char) + char.setter_callback = setter + + with patch.object(Characteristic, "notify") as notify: + _write(driver, acc, char, **{HAP_REPR_WRITE_RESPONSE: True}) + await asyncio.wait_for(setter.done.wait(), ASYNC_TIMEOUT) + await asyncio.sleep(0) + + notify.assert_not_called() + + class _StubbornSetter: """An async setter that ignores its cancellation and fails afterwards.""" def __init__(self) -> None: - self.failed = asyncio.Event() + self.started = asyncio.Event() async def __call__(self, value: int) -> None: + self.started.set() try: await asyncio.sleep(10) except asyncio.CancelledError: await asyncio.sleep(0) - self.failed.set() raise ValueError("failed after the cancellation") from None -async def test_setter_failing_after_its_timeout_is_retrieved( - driver: AccessoryDriver, caplog: pytest.LogCaptureFixture +async def _write_and_cancel( + driver: AccessoryDriver, query: dict[str, Any], setter: _StubbornSetter +) -> None: + """Start a controller write and cancel it once the setter runs.""" + result = driver._set_characteristics_for_controller(query, WRITER, ASYNC_TIMEOUT) + assert inspect.isawaitable(result) + task = asyncio.ensure_future(result) + await asyncio.wait_for(setter.started.wait(), ASYNC_TIMEOUT) + task.cancel() + await asyncio.gather(task, return_exceptions=True) + + +async def _write_until_timeout( + driver: AccessoryDriver, query: dict[str, Any], setter: _StubbornSetter ) -> None: - """A setter that runs on after its timeout has its error logged, not lost.""" - caplog.set_level(logging.DEBUG, "pyhap.accessory_driver") + """Make a controller write whose setter outlives the timeout.""" + result = driver._set_characteristics_for_controller(query, WRITER, 0.05) + assert inspect.isawaitable(result) + await result + + +@pytest.mark.parametrize( + "run", [_write_until_timeout, _write_and_cancel], ids=["timeout", "cancelled"] +) +async def test_setter_failing_after_its_cancellation_is_retrieved( + driver: AccessoryDriver, run: Any +) -> None: + """A setter that runs on after its cancellation has its error logged.""" acc, char, _ = _add_bridged_or_standalone(driver, ["pw"], False) setter = _StubbornSetter() char.setter_callback = setter + logged = asyncio.Event() query = { HAP_REPR_CHARS: [ { @@ -1309,13 +1518,13 @@ async def test_setter_failing_after_its_timeout_is_retrieved( ] } - result = driver._set_characteristics_for_controller(query, WRITER, 0.05) - assert inspect.isawaitable(result) - await result - await asyncio.wait_for(setter.failed.wait(), ASYNC_TIMEOUT) - await asyncio.sleep(0) + with patch.object( + accessory_driver.logger, "debug", side_effect=lambda *_, **__: logged.set() + ) as log_debug: + await run(driver, query, setter) + await asyncio.wait_for(logged.wait(), ASYNC_TIMEOUT) - assert "Setter failed after it was cancelled" in caplog.text + assert log_debug.call_args.args == ("Setter failed after it was cancelled",) @pytest.mark.parametrize("flag", [True, 1], ids=["true", "one"])