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/22] 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/22] 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/22] 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/22] 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/22] 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/22] 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/22] 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/22] 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/22] 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/22] 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/22] 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/22] 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/22] 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 af8d2c02451179dae55f762e48a15e4ef6d00612 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Mon, 9 Nov 2026 07:16:35 +0100 Subject: [PATCH 14/22] Keep the Setup Endpoints answer per connection A controller writes Setup Endpoints and reads the answer back. The answer carries the session keys, so it now stays with the connection that wrote, like a write response. A characteristic with read_back_response keeps what its setter returns for the writing connection, until it writes again or closes. Other connections and /accessories read the characteristic's own value, and the written request is not left as that value either. The same now holds for any write to a readable write response characteristic, with or without r. set_endpoints() returns its answer instead of setting the value. --- pyhap/accessory_driver.py | 48 +++++++++++++++++++++-------- pyhap/camera.py | 15 +++++----- pyhap/characteristic.py | 5 ++++ tests/test_accessory_driver.py | 55 ++++++++++++++++++++++++++++++---- tests/test_camera.py | 40 ++++++++++++++++++++++--- 5 files changed, 133 insertions(+), 30 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index 665ae5ba..aaab755d 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -1256,12 +1256,16 @@ def _run_characteristic_writes( 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. + # request of a readable write response or read back characteristic + # is not shared, with or without r and 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 + ( + HAP_PERMISSION_WRITE_RESPONSE in char.properties[PROP_PERMISSIONS] + or char.read_back_response + ) and value is not None and HAP_PERMISSION_READ in char.properties[PROP_PERMISSIONS] ) @@ -1285,9 +1289,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)) - elif set_result_value is not None and write_response: - result[HAP_REPR_VALUE] = self._store_write_response( - char, set_result_value, client_addr + elif set_result_value is not None: + self._keep_response( + result, char, set_result_value, write_response, client_addr ) results[aid][iid] = result @@ -1363,14 +1367,31 @@ async def _async_finish_set_characteristics( HAP_SERVER_STATUS.SERVICE_COMMUNICATION_FAILURE ) continue - if outcome is not None and write_response: + if outcome is not None: # 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 - ) + self._keep_response(result, char, outcome, write_response, client_addr) return _set_characteristics_response(results) + def _keep_response( + self, + result: dict[str, Any], + char: Characteristic, + response: HAPValue, + write_response: bool, + client_addr: tuple[str, int] | None, + ) -> None: + """Keep what a setter returned as a write response or read back value.""" + if write_response: + result[HAP_REPR_VALUE] = self._store_write_response( + char, response, client_addr + ) + elif ( + char.read_back_response + and HAP_PERMISSION_READ in char.properties[PROP_PERMISSIONS] + ): + self._store_write_response(char, response, client_addr) + 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) @@ -1389,9 +1410,10 @@ def _store_write_response( 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. + which the write leaves as it was, also for a write without ``r``. A + characteristic that also has ``ev`` notifies the written value as any + write does. A read back characteristic keeps the response the same + way, without a write response. """ readable = HAP_PERMISSION_READ in char.properties[PROP_PERMISSIONS] try: diff --git a/pyhap/camera.py b/pyhap/camera.py index 4caccd4e..8daca080 100644 --- a/pyhap/camera.py +++ b/pyhap/camera.py @@ -590,12 +590,14 @@ def _create_stream_management( "SelectedRTPStreamConfiguration", setter_callback=self.set_selected_stream_configuration, ) - management.configure_char( + setup_endpoints = management.configure_char( "SetupEndpoints", setter_callback=lambda value: self.set_endpoints( value, stream_idx=stream_idx ), ) + # The answer has the session's SRTP keys, only its controller reads it. + setup_endpoints.read_back_response = True return management def _default_recording_configs( @@ -1254,11 +1256,12 @@ def set_streaming_available(self, stream_idx: int) -> None: self._streaming_status[stream_idx] = STREAMING_STATUS["AVAILABLE"] self._management[stream_idx].get_characteristic("StreamingStatus").notify() - def set_endpoints(self, value: str, stream_idx: int | None = None) -> None: + def set_endpoints(self, value: str, stream_idx: int | None = None) -> Any: """Configure streaming endpoints. - Called when iOS sets the SetupEndpoints ``Characteristic``. The endpoint - information for the camera should be set as the current value of SetupEndpoints. + Called when iOS sets the SetupEndpoints ``Characteristic``. Returns the + endpoint information of the camera, which only the connection that + wrote reads back. :param value: The base64-encoded stream session details in TLV format. :param value: ``str`` @@ -1412,9 +1415,7 @@ def set_endpoints(self, value: str, stream_idx: int | None = None) -> None: "audio_backchannel": audio_backchannel, } - self._management[stream_idx].get_characteristic("SetupEndpoints").set_value( - response_tlv - ) + return response_tlv async def run(self) -> None: """Start the HDS listener when recording transport is available.""" diff --git a/pyhap/characteristic.py b/pyhap/characteristic.py index 4472638b..b22ac87d 100644 --- a/pyhap/characteristic.py +++ b/pyhap/characteristic.py @@ -160,6 +160,7 @@ class Characteristic: "_always_null", "_setter_addr_cache", "_setter_addr_cache_for", + "read_back_response", ) def __init__( @@ -216,6 +217,10 @@ def __init__( self._loader_display_name: str | None = None self._to_hap_cache_with_value: dict[str, Any] | None = None self._to_hap_cache: dict[str, Any] | None = None + # For a control point a controller writes and then reads, such as + # Setup Endpoints: the setter's return value is what the writing + # connection reads back, like a write response. + self.read_back_response = False @property def display_name(self) -> str | None: diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index 82a2ccde..9c92a4b1 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -1333,14 +1333,14 @@ async def test_write_response_dropped_on_disconnect(driver: AccessoryDriver) -> def test_plain_write_replaces_write_response(driver: AccessoryDriver) -> None: - """A write without ``r`` drops the writer's former response.""" + """A write without ``r`` drops the writer's former response, privately too.""" 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 + assert _read(driver, char_id, WRITER) == 0 + assert _read(driver, char_id, OTHER_CONTROLLER) == 0 @pytest.mark.parametrize( @@ -1451,15 +1451,15 @@ def test_write_response_without_client_not_kept(driver: AccessoryDriver) -> None pytest.param({HAP_REPR_WRITE_RESPONSE: False}, id="declined"), ], ) -def test_written_value_readable_without_write_response( +def test_written_value_private_without_write_response( driver: AccessoryDriver, reader: Any, query: dict[str, Any] ) -> None: - """Without a write response the written value is read by every controller.""" + """Without a write response the written value is not left readable either.""" 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 + assert _read(driver, char_id, reader) == 0 def test_service_callbacks(driver: AccessoryDriver) -> None: @@ -2570,3 +2570,46 @@ def test_prepare_bounds_prepared_writes(driver: AccessoryDriver) -> None: # The earliest-registered pids were evicted, the latest kept. assert 0 not in pending assert MAX_PREPARED_WRITES_PER_CLIENT + 4 in pending + + +def _add_read_back_char(driver: AccessoryDriver) -> tuple[Accessory, Characteristic]: + """Add a pr and pw char that answers a write with 7 to read back.""" + acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw"], False) + char.getter_callback = None + char.setter_callback = MagicMock(return_value=7) + char.read_back_response = True + return acc, char + + +@pytest.mark.parametrize( + ("reader", "expected"), + [ + pytest.param(WRITER, 7, id="writer"), + pytest.param(OTHER_CONTROLLER, 0, id="other-controller"), + ], +) +def test_read_back_response_only_for_writer( + driver: AccessoryDriver, reader: Any, expected: int +) -> None: + """Only the writing controller reads back what the setter answered.""" + acc, char = _add_read_back_char(driver) + + assert _write(driver, acc, char, **{HAP_REPR_VALUE: 5}) is None + + assert _read(driver, f"{acc.aid}.{acc.iid_manager.get_iid(char)}", reader) == ( + expected + ) + 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_async_read_back_response(driver: AccessoryDriver) -> None: + """A coroutine setter's answer is read back, not sent as a write response.""" + acc, char = _add_read_back_char(driver) + char.setter_callback = _async_return_one + + assert await _controller_write(driver, acc, char, **{HAP_REPR_VALUE: 5}) is None + + char_id = f"{acc.aid}.{acc.iid_manager.get_iid(char)}" + assert _read(driver, char_id, WRITER) == 1 + assert _read(driver, char_id, OTHER_CONTROLLER) == 0 diff --git a/tests/test_camera.py b/tests/test_camera.py index 9d81af8b..d70e7fd0 100644 --- a/tests/test_camera.py +++ b/tests/test_camera.py @@ -90,9 +90,9 @@ def test_setup_endpoints(mock_driver: Any) -> None: management = acc.get_service("CameraRTPStreamManagement") assert management is not None setup_endpoints = management.get_characteristic("SetupEndpoints") - setup_endpoints.client_update_value(set_endpoint_req) + response = setup_endpoints.client_update_value(set_endpoint_req) - assert setup_endpoints.get_value()[:171] == set_endpoint_res[:171] + assert response[:171] == set_endpoint_res[:171] def test_setup_endpoints_does_not_log_srtp_keys( @@ -270,13 +270,13 @@ def test_setup_endpoints_reports_real_port_when_talkback_enabled( management = acc.get_service("CameraRTPStreamManagement") assert management is not None setup_endpoints = management.get_characteristic("SetupEndpoints") - setup_endpoints.client_update_value(set_endpoint_req) + response = setup_endpoints.client_update_value(set_endpoint_req) session_id = list(acc.sessions)[0] receiver = acc.sessions[session_id]["audio_backchannel"] assert receiver is not None - reported_port = _accessory_audio_port(setup_endpoints.get_value()) + reported_port = _accessory_audio_port(response) assert reported_port == receiver.local_port assert reported_port != _controller_audio_port(set_endpoint_req) @@ -420,3 +420,35 @@ async def subprocess_exec(*_args: Any, **_kwargs: Any) -> Mock: ): assert await acc.start_stream(session_info, safe_config) is True exec_mock.assert_called_once() + + +def test_setup_endpoints_read_back_by_writer_only(driver: Any) -> None: + """Test only the controller that set up a session reads its answer.""" + acc = camera.Camera(_OPTIONS, driver, "Camera") + driver.add_accessory(acc) + management = acc.get_service("CameraRTPStreamManagement") + assert management is not None + char = management.get_characteristic("SetupEndpoints") + char_id = f"{acc.aid}.{acc.iid_manager.get_iid(char)}" + request = ( + "ARCszGzBBWNFFY2pdLRQkAaRAxoBAQACDTE5Mi4xNjguMS4xMTQDAjPFBAKs1gQ" + "lAhDYlmCkyTBZQfxqFS3OnxVOAw4bQZm5NuoQjyanlqWA0QEBAAUlAhAKRPSRVa" + "qGeNmESTIojxNiAw78WkjTLtGv0waWnLo9gQEBAA==" + ) + query = { + "characteristics": [ + {"aid": acc.aid, "iid": acc.iid_manager.get_iid(char), "value": request} + ] + } + + assert driver.set_characteristics(query, ("192.168.1.114", 50000)) is None + + (writer,) = driver.get_characteristics([char_id], ("192.168.1.114", 50000))[ + "characteristics" + ] + (other,) = driver.get_characteristics([char_id], ("192.168.1.115", 50000))[ + "characteristics" + ] + assert writer["value"][:20] == "ARCszGzBBWNFFY2pdLRQ" + assert other["value"] == "" + assert char.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 15/22] 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 16/22] 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 17/22] 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 1b10be97f91cad85558ef6b82bf4a4aa5efdd6b9 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 14:37:46 +0100 Subject: [PATCH 18/22] Keep read back answers private, also when the setter sets them A subclass that still sets the Setup Endpoints answer as the value made it readable for every controller. Whatever the setter of a read back characteristic leaves in the value now goes to the writer's store and the former value comes back. A plain write to another write response characteristic stays readable for everyone, as in #535. The read back test now checks the answer's status instead of the session ID. --- pyhap/accessory_driver.py | 20 +++++++++++----- tests/test_accessory_driver.py | 39 +++++++++++++++++++++++++----- tests/test_camera.py | 44 +++++++++++++++++++++++++++++++++- 3 files changed, 90 insertions(+), 13 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index 949836e8..8206e13e 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -1270,10 +1270,7 @@ def _run_characteristic_writes( if value is not None: self._write_responses.get(client_addr, {}).pop(char, None) private_write = ( - ( - HAP_PERMISSION_WRITE_RESPONSE in char.properties[PROP_PERMISSIONS] - or char.read_back_response - ) + (write_response or char.read_back_response) and value is not None and HAP_PERMISSION_READ in char.properties[PROP_PERMISSIONS] ) @@ -1291,8 +1288,19 @@ def _run_characteristic_writes( # 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 + # Whatever the setter of a read back characteristic leaves in the + # value is an answer for the writer only, e.g. from a subclass that + # still sets the Setup Endpoints answer itself. + if private_write: + own_value = char.value + if char.read_back_response: + if set_result_value is None and own_value != _stored_value( + char, value + ): + set_result_value = own_value + char.value = previous_value + elif own_value == _stored_value(char, value): + char.value = previous_value result: dict[str, Any] = {HAP_REPR_STATUS: set_result} if inspect.iscoroutine(set_result_value): diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index c3ce2c6f..1aa0d2f2 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -1333,14 +1333,14 @@ async def test_write_response_dropped_on_disconnect(driver: AccessoryDriver) -> def test_plain_write_replaces_write_response(driver: AccessoryDriver) -> None: - """A write without ``r`` drops the writer's former response, privately too.""" + """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) == 0 - assert _read(driver, char_id, OTHER_CONTROLLER) == 0 + assert _read(driver, char_id, WRITER) == 9 + assert _read(driver, char_id, OTHER_CONTROLLER) == 9 @pytest.mark.parametrize( @@ -1479,15 +1479,15 @@ def test_write_response_without_client_not_kept(driver: AccessoryDriver) -> None pytest.param({HAP_REPR_WRITE_RESPONSE: False}, id="declined"), ], ) -def test_written_value_private_without_write_response( +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 not left readable either.""" + """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) == 0 + assert _read(driver, char_id, reader) == 5 def test_service_callbacks(driver: AccessoryDriver) -> None: @@ -2631,6 +2631,33 @@ def test_read_back_response_only_for_writer( assert acc_rep[HAP_REPR_SERVICES][-1][HAP_REPR_CHARS][0][HAP_REPR_VALUE] == 0 +@pytest.mark.parametrize( + ("reader", "expected"), + [ + pytest.param(WRITER, 42, id="writer"), + pytest.param(OTHER_CONTROLLER, 0, id="other-controller"), + ], +) +def test_read_back_value_set_by_setter_only_for_writer( + driver: AccessoryDriver, reader: Any, expected: int +) -> None: + """A value the setter sets itself on a read back char stays with the writer.""" + acc, char = _add_read_back_char(driver) + + def _setter(value: int) -> None: + char.set_value(42) + + char.setter_callback = _setter + + assert _write(driver, acc, char, **{HAP_REPR_VALUE: 5}) is None + + assert _read(driver, f"{acc.aid}.{acc.iid_manager.get_iid(char)}", reader) == ( + expected + ) + 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_async_read_back_response(driver: AccessoryDriver) -> None: """A coroutine setter's answer is read back, not sent as a write response.""" acc, char = _add_read_back_char(driver) diff --git a/tests/test_camera.py b/tests/test_camera.py index d70e7fd0..3af026ce 100644 --- a/tests/test_camera.py +++ b/tests/test_camera.py @@ -449,6 +449,48 @@ def test_setup_endpoints_read_back_by_writer_only(driver: Any) -> None: (other,) = driver.get_characteristics([char_id], ("192.168.1.115", 50000))[ "characteristics" ] - assert writer["value"][:20] == "ARCszGzBBWNFFY2pdLRQ" + # The writer reads the answer, which carries a status, not its request. + answer = tlv.decode(writer["value"], from_base64=True) + assert answer[b"\x01"] == tlv.decode(request, from_base64=True)[b"\x01"] + assert answer[b"\x02"] == b"\x00" + assert other["value"] == "" + assert char.value == "" + + +def test_setup_endpoints_set_by_subclass_read_back_by_writer_only( + driver: Any, +) -> None: + """Test an answer a subclass sets as the value stays with the writer.""" + + class _Camera(camera.Camera): + def set_endpoints(self, value: str, stream_idx: int | None = None) -> None: + answer = super().set_endpoints(value, stream_idx) + management = self.get_service("CameraRTPStreamManagement") + assert management is not None + management.get_characteristic("SetupEndpoints").set_value(answer) + + acc = _Camera(_OPTIONS, driver, "Camera") + driver.add_accessory(acc) + management = acc.get_service("CameraRTPStreamManagement") + assert management is not None + char = management.get_characteristic("SetupEndpoints") + iid = acc.iid_manager.get_iid(char) + request = ( + "ARCszGzBBWNFFY2pdLRQkAaRAxoBAQACDTE5Mi4xNjguMS4xMTQDAjPFBAKs1gQ" + "lAhDYlmCkyTBZQfxqFS3OnxVOAw4bQZm5NuoQjyanlqWA0QEBAAUlAhAKRPSRVa" + "qGeNmESTIojxNiAw78WkjTLtGv0waWnLo9gQEBAA==" + ) + query = {"characteristics": [{"aid": acc.aid, "iid": iid, "value": request}]} + + assert driver.set_characteristics(query, ("192.168.1.114", 50000)) is None + + char_id = f"{acc.aid}.{iid}" + (writer,) = driver.get_characteristics([char_id], ("192.168.1.114", 50000))[ + "characteristics" + ] + (other,) = driver.get_characteristics([char_id], ("192.168.1.115", 50000))[ + "characteristics" + ] + assert tlv.decode(writer["value"], from_base64=True)[b"\x02"] == b"\x00" assert other["value"] == "" assert char.value == "" 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 19/22] 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 18cbcbbef6f74f2a914735af91b8a4ca4a180139 Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 16:16:28 +0100 Subject: [PATCH 20/22] Answer a Setup Endpoints refused while streaming is off only to the writer --- pyhap/camera.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/pyhap/camera.py b/pyhap/camera.py index 1554e347..70675bb2 100644 --- a/pyhap/camera.py +++ b/pyhap/camera.py @@ -1678,16 +1678,14 @@ def set_endpoints(self, value: str, stream_idx: int | None = None) -> Any: session_id = UUID(bytes=objs[SETUP_TYPES["SESSION_ID"]]) if not self.streaming_allowed: logger.info("[%s] Streaming is turned off, refusing it", session_id) - self._management[stream_idx].get_characteristic("SetupEndpoints").set_value( - tlv.encode( - SETUP_TYPES["SESSION_ID"], - session_id.bytes, - SETUP_TYPES["STATUS"], - SETUP_STATUS["ERROR"], - to_base64=True, - ) + # Like an answer, the refusal is only for the writer. + return tlv.encode( + SETUP_TYPES["SESSION_ID"], + session_id.bytes, + SETUP_TYPES["STATUS"], + SETUP_STATUS["ERROR"], + to_base64=True, ) - return # Extract address info address_tlv = objs[SETUP_TYPES["ADDRESS"]] From b28c8c827358f838963669149da44985d0fe2aae Mon Sep 17 00:00:00 2001 From: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Date: Tue, 10 Nov 2026 19:34:08 +0100 Subject: [PATCH 21/22] Keep setting the Setup Endpoints answer as the value A subclass that wraps set_endpoints, calls super() and returns nothing got no answer once set_endpoints only returned it. set_endpoints sets the answer, or a refusal, as the value again and returns it. The driver keeps what a read back setter leaves as the value for the writer only and puts the shared value back. Neither the former value of a refused write nor the request a setter left alone counts as an answer. An async setter must return the answer, a value it sets itself is shared. Correct the write response docstring for a write without r, type the Setup Endpoints helpers, document read_back_response and share the Setup Endpoints request of the camera tests. --- pyhap/accessory_driver.py | 19 +-- pyhap/camera.py | 38 +++-- pyhap/characteristic.py | 7 + tests/test_accessory_driver.py | 37 +++++ tests/test_camera.py | 249 ++++++++++++++++++--------------- 5 files changed, 219 insertions(+), 131 deletions(-) diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py index b3719d0e..032c4286 100644 --- a/pyhap/accessory_driver.py +++ b/pyhap/accessory_driver.py @@ -1368,13 +1368,16 @@ def _run_characteristic_writes( # client_update_value stored the request as the value, so put # back the former one unless the setter set a value of its own. # Whatever the setter of a read back characteristic leaves in the - # value is an answer for the writer only, e.g. from a subclass that - # still sets the Setup Endpoints answer itself. + # value is an answer for the writer only, e.g. the Setup Endpoints + # answer of a subclass that calls super() and returns nothing. + # Neither the former value of a refused write nor the request the + # setter left alone is an answer. if private_write: own_value = char.value if char.read_back_response: - if set_result_value is None and own_value != _stored_value( - char, value + if set_result_value is None and own_value not in ( + previous_value, + _stored_value(char, value), ): set_result_value = own_value char.value = previous_value @@ -1543,10 +1546,10 @@ def _store_write_response( 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, also for a write without ``r``. A - characteristic that also has ``ev`` notifies the written value as any - write does. A read back characteristic keeps the response the same - way, without a write response. + which the write leaves as it was. A write without ``r`` stays readable + for everyone and notifies ``ev`` subscribers as any write does, unless + the characteristic has ``read_back_response``, which keeps the response + the same way without a write response. """ readable = HAP_PERMISSION_READ in char.properties[PROP_PERMISSIONS] try: diff --git a/pyhap/camera.py b/pyhap/camera.py index 6a0ac605..8de494d0 100644 --- a/pyhap/camera.py +++ b/pyhap/camera.py @@ -7,8 +7,8 @@ [0. Read the current streaming status] 1. Sets the SetupEndpoints characteristic to notify the camera about its IP address, selected security parameters, etc. -2. The camera responds to the above by setting the SetupEndpoints with its IP address, -etc. +2. The camera answers the write with its IP address, etc., and the client reads the +answer back from SetupEndpoints. 3. The client sets the SelectedRTPStreamConfiguration characteristic to notify the camera of its prefered audio and video configuration and to initiate the start of the streaming. @@ -349,7 +349,7 @@ def _decode_setup_endpoints( ) -def _setup_endpoints_status(session_id: UUID, status: bytes) -> Any: +def _setup_endpoints_status(session_id: UUID, status: bytes) -> str: """Encode a Setup Endpoints answer that has only a status.""" return tlv.encode( SETUP_TYPES["SESSION_ID"], @@ -1750,15 +1750,22 @@ def set_streaming_available(self, stream_idx: int) -> None: self._streaming_status[stream_idx] = STREAMING_STATUS["AVAILABLE"] self._management[stream_idx].get_characteristic("StreamingStatus").notify() - def set_endpoints(self, value: str, stream_idx: int | None = None) -> Any: + def set_endpoints(self, value: str, stream_idx: int | None = None) -> str: """Configure streaming endpoints. - Called when iOS sets the SetupEndpoints ``Characteristic``. Returns the - endpoint information of the camera, which only the connection that - wrote reads back. + Called when iOS sets the SetupEndpoints ``Characteristic``. Sets the + endpoint information of the camera, or a refusal, as the value of + SetupEndpoints and returns it. A subclass that calls ``super()`` and + returns nothing still answers. For a controller's write the driver + keeps the answer for the connection that wrote and puts the former + value back. An async setter must return the answer and must not set + the value itself, a value it sets is shared with every controller. A + direct ``client_update_value`` leaves the answer as the value. :param value: The base64-encoded stream session details in TLV format. :param value: ``str`` + :return: The base64-encoded answer in TLV format. + :rtype: ``str`` """ if stream_idx is None: stream_idx = 0 @@ -1777,11 +1784,15 @@ def set_endpoints(self, value: str, stream_idx: int | None = None) -> Any: except (IndexError, KeyError, ValueError, struct.error) as ex: logger.warning("[%s] Refusing the endpoints: %s", session_id, ex) # Like an answer, the refusal is only for the writer. - return _setup_endpoints_status(session_id, SETUP_STATUS["ERROR"]) + return self._answer_endpoints( + stream_idx, _setup_endpoints_status(session_id, SETUP_STATUS["ERROR"]) + ) if not self.streaming_allowed: logger.info("[%s] Streaming is turned off, refusing it", session_id) - return _setup_endpoints_status(session_id, SETUP_STATUS["ERROR"]) + return self._answer_endpoints( + stream_idx, _setup_endpoints_status(session_id, SETUP_STATUS["ERROR"]) + ) # Configure the SetupEndpoints response @@ -1879,7 +1890,14 @@ def set_endpoints(self, value: str, stream_idx: int | None = None) -> Any: "audio_backchannel": audio_backchannel, } - return response_tlv + return self._answer_endpoints(stream_idx, response_tlv) + + def _answer_endpoints(self, stream_idx: int, answer: str) -> str: + """Set the Setup Endpoints answer as the value and return it.""" + self._management[stream_idx].get_characteristic("SetupEndpoints").set_value( + answer, should_notify=False + ) + return answer async def run(self) -> None: """Start the HDS listener when recording transport is available.""" diff --git a/pyhap/characteristic.py b/pyhap/characteristic.py index 21ee2299..e5739cd1 100644 --- a/pyhap/characteristic.py +++ b/pyhap/characteristic.py @@ -148,6 +148,13 @@ class Characteristic: the current temperature. Characteristics are contained in services. Each characteristic has a unique type UUID and a set of properties, like format, min and max values, valid values and others. + + Set ``read_back_response`` to True for a readable control point that a + controller writes and then reads, such as Setup Endpoints: what the + setter returns or leaves as the value is read back only by the connection + that wrote, and the shared value stays as it was. An async setter must + return the answer and must not set the value itself, a value it sets is + shared with every controller. """ __slots__ = ( diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index 47499589..13251a16 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -2959,6 +2959,23 @@ def test_read_back_response_only_for_writer( assert acc_rep[HAP_REPR_SERVICES][-1][HAP_REPR_CHARS][0][HAP_REPR_VALUE] == 0 +def test_read_back_prefers_the_returned_answer(driver: AccessoryDriver) -> None: + """The answer a setter returns wins over a value it sets itself.""" + acc, char = _add_read_back_char(driver) + + def _setter(value: int) -> int: + char.set_value(42) + return 7 + + char.setter_callback = _setter + + assert _write(driver, acc, char, **{HAP_REPR_VALUE: 5}) is None + + char_id = f"{acc.aid}.{acc.iid_manager.get_iid(char)}" + assert _read(driver, char_id, WRITER) == 7 + assert _read(driver, char_id, OTHER_CONTROLLER) == 0 + + @pytest.mark.parametrize( ("reader", "expected"), [ @@ -2998,6 +3015,26 @@ async def test_async_read_back_response(driver: AccessoryDriver) -> None: assert _read(driver, char_id, OTHER_CONTROLLER) == 0 +@pytest.mark.parametrize( + ("value", "setter"), + [ + pytest.param("notanint", MagicMock(return_value=None), id="invalid-value"), + pytest.param(5, MagicMock(return_value=None), id="setter-does-nothing"), + ], +) +def test_read_back_without_answer_keeps_nothing_for_writer( + driver: AccessoryDriver, value: Any, setter: MagicMock +) -> None: + """Neither the former value nor the request is kept as the writer's answer.""" + acc, char = _add_read_back_char(driver) + char.setter_callback = setter + + _write(driver, acc, char, **{HAP_REPR_VALUE: value}) + char.set_value(3) + + assert _read(driver, f"{acc.aid}.{acc.iid_manager.get_iid(char)}", WRITER) == 3 + + CLIENT = ("1.2.3.4", 5) diff --git a/tests/test_camera.py b/tests/test_camera.py index c9acf676..e0ff83a2 100644 --- a/tests/test_camera.py +++ b/tests/test_camera.py @@ -7,13 +7,20 @@ import socket import struct from typing import Any, cast -from unittest.mock import AsyncMock, Mock, patch +from unittest.mock import AsyncMock, Mock, PropertyMock, patch from uuid import UUID import pytest from pyhap import camera, tlv +# A Setup Endpoints request of a controller at 192.168.1.114. +_SETUP_ENDPOINTS_REQUEST = ( + "ARCszGzBBWNFFY2pdLRQkAaRAxoBAQACDTE5Mi4xNjguMS4xMTQDAjPFBAKs1gQ" + "lAhDYlmCkyTBZQfxqFS3OnxVOAw4bQZm5NuoQjyanlqWA0QEBAAUlAhAKRPSRVa" + "qGeNmESTIojxNiAw78WkjTLtGv0waWnLo9gQEBAA==" +) + _OPTIONS = { "stream_count": 4, "video": { @@ -75,12 +82,6 @@ def test_init(mock_driver: Any) -> None: def test_setup_endpoints(mock_driver: Any) -> None: """Test that the SetupEndpoint response is computed correctly""" - set_endpoint_req = ( - "ARCszGzBBWNFFY2pdLRQkAaRAxoBAQACDTE5Mi4xNjguMS4xMTQDAjPFBAKs1gQ" - "lAhDYlmCkyTBZQfxqFS3OnxVOAw4bQZm5NuoQjyanlqWA0QEBAAUlAhAKRPSRVa" - "qGeNmESTIojxNiAw78WkjTLtGv0waWnLo9gQEBAA==" - ) - set_endpoint_res = ( "ARCszGzBBWNFFY2pdLRQkAaRAgEAAxoBAQACDTE5Mi4xNjguMS4yMjYDAjPFBAK" "s1gQlAQEAAhDYlmCkyTBZQfxqFS3OnxVOAw4bQZm5NuoQjyanlqWA0QUlAQEAAh" @@ -91,33 +92,29 @@ def test_setup_endpoints(mock_driver: Any) -> None: management = acc.get_service("CameraRTPStreamManagement") assert management is not None setup_endpoints = management.get_characteristic("SetupEndpoints") - response = setup_endpoints.client_update_value(set_endpoint_req) + response = setup_endpoints.client_update_value(_SETUP_ENDPOINTS_REQUEST) assert response[:171] == set_endpoint_res[:171] + # Without the driver nothing puts the former value back. + assert setup_endpoints.get_value() == response def test_setup_endpoints_does_not_log_srtp_keys( mock_driver: Any, caplog: pytest.LogCaptureFixture ) -> None: """Verify the negotiated SRTP key material is not written to the logs.""" - set_endpoint_req = ( - "ARCszGzBBWNFFY2pdLRQkAaRAxoBAQACDTE5Mi4xNjguMS4xMTQDAjPFBAKs1gQ" - "lAhDYlmCkyTBZQfxqFS3OnxVOAw4bQZm5NuoQjyanlqWA0QEBAAUlAhAKRPSRVa" - "qGeNmESTIojxNiAw78WkjTLtGv0waWnLo9gQEBAA==" - ) - acc = camera.Camera(_OPTIONS, mock_driver, "Camera") management = acc.get_service("CameraRTPStreamManagement") assert management is not None setup_endpoints = management.get_characteristic("SetupEndpoints") with caplog.at_level(logging.DEBUG, logger="pyhap.camera"): - setup_endpoints.client_update_value(set_endpoint_req) + setup_endpoints.client_update_value(_SETUP_ENDPOINTS_REQUEST) # The endpoint configuration was logged at DEBUG level, but without any # of the SRTP master key/salt material in it. assert "Received endpoint configuration:" in caplog.text - key_and_salt = _audio_srtp_key_and_salt(set_endpoint_req) + key_and_salt = _audio_srtp_key_and_salt(_SETUP_ENDPOINTS_REQUEST) assert base64.b64encode(key_and_salt).decode() not in caplog.text assert key_and_salt.hex() not in caplog.text @@ -186,19 +183,13 @@ def test_no_talkback_by_default(mock_driver: Any) -> None: SetupEndpoints keeps echoing the controller's port back (unchanged, pre-existing behavior for accessories that don't opt in). """ - set_endpoint_req = ( - "ARCszGzBBWNFFY2pdLRQkAaRAxoBAQACDTE5Mi4xNjguMS4xMTQDAjPFBAKs1gQ" - "lAhDYlmCkyTBZQfxqFS3OnxVOAw4bQZm5NuoQjyanlqWA0QEBAAUlAhAKRPSRVa" - "qGeNmESTIojxNiAw78WkjTLtGv0waWnLo9gQEBAA==" - ) - acc = camera.Camera(_OPTIONS, mock_driver, "Camera") assert acc.get_service("Speaker") is None management = acc.get_service("CameraRTPStreamManagement") assert management is not None setup_endpoints = management.get_characteristic("SetupEndpoints") - setup_endpoints.client_update_value(set_endpoint_req) + setup_endpoints.client_update_value(_SETUP_ENDPOINTS_REQUEST) session_id = list(acc.sessions)[0] assert acc.sessions[session_id]["audio_backchannel"] is None @@ -260,18 +251,12 @@ def test_setup_endpoints_reports_real_port_when_talkback_enabled( this module fixes: HAP clients had nowhere valid to send the talkback audio to, because the accessory never actually listened anywhere). """ - set_endpoint_req = ( - "ARCszGzBBWNFFY2pdLRQkAaRAxoBAQACDTE5Mi4xNjguMS4xMTQDAjPFBAKs1gQ" - "lAhDYlmCkyTBZQfxqFS3OnxVOAw4bQZm5NuoQjyanlqWA0QEBAAUlAhAKRPSRVa" - "qGeNmESTIojxNiAw78WkjTLtGv0waWnLo9gQEBAA==" - ) - options = dict(_OPTIONS, talkback=True) acc = camera.Camera(options, mock_driver, "Camera") management = acc.get_service("CameraRTPStreamManagement") assert management is not None setup_endpoints = management.get_characteristic("SetupEndpoints") - response = setup_endpoints.client_update_value(set_endpoint_req) + response = setup_endpoints.client_update_value(_SETUP_ENDPOINTS_REQUEST) session_id = list(acc.sessions)[0] receiver = acc.sessions[session_id]["audio_backchannel"] @@ -279,7 +264,7 @@ def test_setup_endpoints_reports_real_port_when_talkback_enabled( reported_port = _accessory_audio_port(response) assert reported_port == receiver.local_port - assert reported_port != _controller_audio_port(set_endpoint_req) + assert reported_port != _controller_audio_port(_SETUP_ENDPOINTS_REQUEST) receiver.stop() @@ -300,18 +285,12 @@ class TalkbackCamera(camera.Camera): def talkback_audio_received(self, session_id: Any, payload: Any) -> None: received.append((session_id, payload)) - set_endpoint_req = ( - "ARCszGzBBWNFFY2pdLRQkAaRAxoBAQACDTE5Mi4xNjguMS4xMTQDAjPFBAKs1gQ" - "lAhDYlmCkyTBZQfxqFS3OnxVOAw4bQZm5NuoQjyanlqWA0QEBAAUlAhAKRPSRVa" - "qGeNmESTIojxNiAw78WkjTLtGv0waWnLo9gQEBAA==" - ) - options = dict(_OPTIONS, talkback=True) acc = TalkbackCamera(options, mock_driver, "Camera") management = acc.get_service("CameraRTPStreamManagement") assert management is not None setup_endpoints = management.get_characteristic("SetupEndpoints") - setup_endpoints.client_update_value(set_endpoint_req) + setup_endpoints.client_update_value(_SETUP_ENDPOINTS_REQUEST) session_id = list(acc.sessions)[0] receiver = acc.sessions[session_id]["audio_backchannel"] @@ -320,7 +299,7 @@ def talkback_audio_received(self, session_id: Any, payload: Any) -> None: # controller is local so the loopback send passes the source filter. receiver._controller_address = "127.0.0.1" - key_and_salt = _audio_srtp_key_and_salt(set_endpoint_req) + key_and_salt = _audio_srtp_key_and_salt(_SETUP_ENDPOINTS_REQUEST) rtp_payload = b"opus-frame-payload" rtp_packet = struct.pack("!BBHII", 0x80, 110, 1, 0, 0x1234) + rtp_payload @@ -423,80 +402,6 @@ async def subprocess_exec(*_args: Any, **_kwargs: Any) -> Mock: exec_mock.assert_called_once() -def test_setup_endpoints_read_back_by_writer_only(driver: Any) -> None: - """Test only the controller that set up a session reads its answer.""" - acc = camera.Camera(_OPTIONS, driver, "Camera") - driver.add_accessory(acc) - management = acc.get_service("CameraRTPStreamManagement") - assert management is not None - char = management.get_characteristic("SetupEndpoints") - char_id = f"{acc.aid}.{acc.iid_manager.get_iid(char)}" - request = ( - "ARCszGzBBWNFFY2pdLRQkAaRAxoBAQACDTE5Mi4xNjguMS4xMTQDAjPFBAKs1gQ" - "lAhDYlmCkyTBZQfxqFS3OnxVOAw4bQZm5NuoQjyanlqWA0QEBAAUlAhAKRPSRVa" - "qGeNmESTIojxNiAw78WkjTLtGv0waWnLo9gQEBAA==" - ) - query = { - "characteristics": [ - {"aid": acc.aid, "iid": acc.iid_manager.get_iid(char), "value": request} - ] - } - - assert driver.set_characteristics(query, ("192.168.1.114", 50000)) is None - - (writer,) = driver.get_characteristics([char_id], ("192.168.1.114", 50000))[ - "characteristics" - ] - (other,) = driver.get_characteristics([char_id], ("192.168.1.115", 50000))[ - "characteristics" - ] - # The writer reads the answer, which carries a status, not its request. - answer = tlv.decode(writer["value"], from_base64=True) - assert answer[b"\x01"] == tlv.decode(request, from_base64=True)[b"\x01"] - assert answer[b"\x02"] == b"\x00" - assert other["value"] == "" - assert char.value == "" - - -def test_setup_endpoints_set_by_subclass_read_back_by_writer_only( - driver: Any, -) -> None: - """Test an answer a subclass sets as the value stays with the writer.""" - - class _Camera(camera.Camera): - def set_endpoints(self, value: str, stream_idx: int | None = None) -> None: - answer = super().set_endpoints(value, stream_idx) - management = self.get_service("CameraRTPStreamManagement") - assert management is not None - management.get_characteristic("SetupEndpoints").set_value(answer) - - acc = _Camera(_OPTIONS, driver, "Camera") - driver.add_accessory(acc) - management = acc.get_service("CameraRTPStreamManagement") - assert management is not None - char = management.get_characteristic("SetupEndpoints") - iid = acc.iid_manager.get_iid(char) - request = ( - "ARCszGzBBWNFFY2pdLRQkAaRAxoBAQACDTE5Mi4xNjguMS4xMTQDAjPFBAKs1gQ" - "lAhDYlmCkyTBZQfxqFS3OnxVOAw4bQZm5NuoQjyanlqWA0QEBAAUlAhAKRPSRVa" - "qGeNmESTIojxNiAw78WkjTLtGv0waWnLo9gQEBAA==" - ) - query = {"characteristics": [{"aid": acc.aid, "iid": iid, "value": request}]} - - assert driver.set_characteristics(query, ("192.168.1.114", 50000)) is None - - char_id = f"{acc.aid}.{iid}" - (writer,) = driver.get_characteristics([char_id], ("192.168.1.114", 50000))[ - "characteristics" - ] - (other,) = driver.get_characteristics([char_id], ("192.168.1.115", 50000))[ - "characteristics" - ] - assert tlv.decode(writer["value"], from_base64=True)[b"\x02"] == b"\x00" - assert other["value"] == "" - assert char.value == "" - - _SESSION_ID = UUID("acccc6cc-1505-6345-158d-a974b4509006") @@ -695,6 +600,124 @@ def test_setup_endpoints_refuses_address_before_streaming_off( } +_WRITER = ("192.168.1.114", 50000) +_OTHER_CONTROLLER = ("192.168.1.115", 50000) + + +class _WrappingCamera(camera.Camera): + """A subclass that wraps set_endpoints and returns nothing.""" + + def set_endpoints( # ty: ignore[invalid-method-override] + self, value: str, stream_idx: int | None = None + ) -> None: + super().set_endpoints(value, stream_idx) + + +class _ReturningCamera(camera.Camera): + """A subclass that wraps set_endpoints and returns the answer.""" + + def set_endpoints(self, value: str, stream_idx: int | None = None) -> str: + return super().set_endpoints(value, stream_idx) + + +class _SettingCamera(camera.Camera): + """A subclass that sets the answer itself, as before, and returns nothing.""" + + def set_endpoints( # ty: ignore[invalid-method-override] + self, value: str, stream_idx: int | None = None + ) -> None: + answer = super().set_endpoints(value, stream_idx) + _setup_endpoints_char(self).set_value(answer) + + +def _setup_endpoints_char(acc: camera.Camera) -> Any: + """Return the Setup Endpoints characteristic of the first stream.""" + management = acc.get_service("CameraRTPStreamManagement") + assert management is not None + return management.get_characteristic("SetupEndpoints") + + +def _setup_endpoints_query( + acc: camera.Camera, request: str = _SETUP_ENDPOINTS_REQUEST +) -> dict[str, Any]: + """Return a controller's write of a Setup Endpoints request.""" + iid = acc.iid_manager.get_iid(_setup_endpoints_char(acc)) + return {"characteristics": [{"aid": acc.aid, "iid": iid, "value": request}]} + + +def _read_setup_endpoints(driver: Any, acc: camera.Camera, client_addr: Any) -> Any: + """Return the Setup Endpoints value ``client_addr`` reads.""" + iid = acc.iid_manager.get_iid(_setup_endpoints_char(acc)) + (rep,) = driver.get_characteristics([f"{acc.aid}.{iid}"], client_addr)[ + "characteristics" + ] + return rep["value"] + + +@pytest.mark.parametrize( + ("request_value", "streaming_allowed", "status"), + [ + pytest.param( + _SETUP_ENDPOINTS_REQUEST, + True, + camera.SETUP_STATUS["SUCCESS"], + id="answer", + ), + pytest.param( + _setup_request(b"192.168.1.114 x", False), + True, + camera.SETUP_STATUS["ERROR"], + id="malformed", + ), + pytest.param( + _SETUP_ENDPOINTS_REQUEST, + False, + camera.SETUP_STATUS["ERROR"], + id="streaming-off", + ), + ], +) +@pytest.mark.parametrize( + "camera_cls", + [camera.Camera, _WrappingCamera, _ReturningCamera, _SettingCamera], + ids=["camera", "wrapping-subclass", "returning-subclass", "setting-subclass"], +) +def test_setup_endpoints_read_back_by_writer_only( + driver: Any, + camera_cls: type[camera.Camera], + request_value: str, + streaming_allowed: bool, + status: bytes, +) -> None: + """Test only the controller that set up a session reads its answer.""" + acc = camera_cls(_OPTIONS, driver, "Camera") + driver.add_accessory(acc) + char = _setup_endpoints_char(acc) + + with patch.object( + camera.Camera, + "streaming_allowed", + new_callable=PropertyMock, + return_value=streaming_allowed, + ): + assert ( + driver.set_characteristics( + _setup_endpoints_query(acc, request_value), _WRITER + ) + is None + ) + + # The writer reads the answer, which carries a status, not its request. + answer = tlv.decode(_read_setup_endpoints(driver, acc, _WRITER), from_base64=True) + session_id = camera.SETUP_TYPES["SESSION_ID"] + assert answer[session_id] == tlv.decode(request_value, from_base64=True)[session_id] + assert answer[camera.SETUP_TYPES["STATUS"]] == status + assert _read_setup_endpoints(driver, acc, _OTHER_CONTROLLER) == "" + assert char.value == "" + # /accessories shows the shared value. + assert char.to_HAP()["value"] == "" + + @pytest.mark.asyncio async def test_stop_stream_process_already_exited(mock_driver: Any) -> None: """Verify stopping a stream whose ffmpeg already exited does not raise.""" From 8ff458a61579650057e9143b5e2eb1d9d795c75a Mon Sep 17 00:00:00 2001 From: Ivan Kalchev <25887324+ikalchev@users.noreply.github.com> Date: Tue, 10 Nov 2026 21:13:11 +0100 Subject: [PATCH 22/22] Use incomaptible-method-override instead of invalid- ty was upgraded today --- tests/test_camera.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_camera.py b/tests/test_camera.py index e0ff83a2..62bf32b8 100644 --- a/tests/test_camera.py +++ b/tests/test_camera.py @@ -607,7 +607,7 @@ def test_setup_endpoints_refuses_address_before_streaming_off( class _WrappingCamera(camera.Camera): """A subclass that wraps set_endpoints and returns nothing.""" - def set_endpoints( # ty: ignore[invalid-method-override] + def set_endpoints( # ty: ignore[incompatible-method-override] self, value: str, stream_idx: int | None = None ) -> None: super().set_endpoints(value, stream_idx) @@ -623,7 +623,7 @@ def set_endpoints(self, value: str, stream_idx: int | None = None) -> str: class _SettingCamera(camera.Camera): """A subclass that sets the answer itself, as before, and returns nothing.""" - def set_endpoints( # ty: ignore[invalid-method-override] + def set_endpoints( # ty: ignore[incompatible-method-override] self, value: str, stream_idx: int | None = None ) -> None: answer = super().set_endpoints(value, stream_idx)