diff --git a/repeater/handler_helpers/room_server.py b/repeater/handler_helpers/room_server.py index c148cf9..489d8fa 100644 --- a/repeater/handler_helpers/room_server.py +++ b/repeater/handler_helpers/room_server.py @@ -399,6 +399,15 @@ class RoomServer: # (from before this limit was enforced on write) exceeds # MAX_POST_TEXT_LEN bytes -- truncate at a UTF-8 codepoint boundary. message_text = post["message_text"] + # The body is a C string on the wire: pushPostToClient sizes it with + # `strlen(post.text)` for both the payload and the expected ACK. A + # post stored with an interior NUL (the web API accepts one) would + # otherwise be transmitted and hashed whole, while the receiver + # stops at that NUL -- so its ACK would never match ours and the + # push would retry until it hit the failure backoff. + nul = message_text.find("\x00") + if nul >= 0: + message_text = message_text[:nul] if len(message_text.encode("utf-8")) > MAX_POST_TEXT_LEN: message_text = _truncate_utf8(message_text, MAX_POST_TEXT_LEN) message_bytes = message_text.encode("utf-8") diff --git a/repeater/handler_helpers/text.py b/repeater/handler_helpers/text.py index 1ff2cc8..c26319e 100644 --- a/repeater/handler_helpers/text.py +++ b/repeater/handler_helpers/text.py @@ -107,6 +107,12 @@ def _may_ack(identity_type, client, txt_type, sender_timestamp) -> bool: A retry (equal timestamp) *is* acknowledged by both -- firmware suppresses the work, not the answer -- so only a strictly older stamp fails here. + This predicts *eligibility*, not the outcome. A post that clears the role + and replay checks and is then refused by ``add_post`` -- openHop's own + per-client rate limit, or a database error, neither of which firmware has -- + is still acknowledged. Closing that would mean feeding the store result back + to an ACK the core handler has already scheduled. + ``txt_type is None`` means a core too old to publish it; there is nothing to judge, so keep the previous behaviour rather than silently going quiet. """ diff --git a/tests/test_handler_helpers_path_protocol_text.py b/tests/test_handler_helpers_path_protocol_text.py index 64b1976..c4d81fe 100644 --- a/tests/test_handler_helpers_path_protocol_text.py +++ b/tests/test_handler_helpers_path_protocol_text.py @@ -1,3 +1,4 @@ +import asyncio import struct import time from types import SimpleNamespace @@ -5,7 +6,9 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest from openhop_core.node.handlers.result import HandlerResult +from openhop_core.node.handlers.text import TXT_ACK_DELAY_MS from openhop_core.protocol import Identity, LocalIdentity +from openhop_core.protocol.constants import PAYLOAD_TYPE_ACK from openhop_core.protocol.packet_builder import PacketBuilder from repeater.handler_helpers.acl import ( @@ -16,7 +19,7 @@ from repeater.handler_helpers.acl import ( ) from repeater.handler_helpers.path import PathHelper from repeater.handler_helpers.protocol_request import ProtocolRequestHelper -from repeater.handler_helpers.text import TextHelper, _may_ack +from repeater.handler_helpers.text import CORE_ACCEPTS_ACK_POLICY, TextHelper, _may_ack # --------------------------------------------------------------------------- # Real-crypto collision scaffolding (BUG-053 "try all local candidates"). @@ -602,7 +605,9 @@ async def test_room_dispatches_on_type_not_text(txt_type, is_command): "get status" ran as a command rather than being posted, and a CLI command whose text was not in the prefix list was published to the room. - Both cases use the *same* text, so only the type can decide. + Both cases use the *same* text, so only the type can decide. The text is + one `_is_cli_command` recognises, which is what makes the PLAIN row the + discriminator; the unrecognised direction is covered separately below. """ helper, room = _room_helper() @@ -617,6 +622,29 @@ async def test_room_dispatches_on_type_not_text(txt_type, is_command): assert room.add_post.await_count == (0 if is_command else 1) +@pytest.mark.asyncio +async def test_room_runs_a_command_whose_text_is_not_a_known_prefix(): + """[fails pre-fix] The other direction: an unrecognised CLI command still runs. + + Deciding from the text published a command to the room whenever it was not + in `_is_cli_command`'s prefix list -- the room's members got to read what + the admin meant to run. Firmware routes every CLI_DATA/CLI_COMMAND from an + admin through handleCommand regardless of what it says + (simple_room_server::onPeerDataRecv). + """ + helper, room = _room_helper() + + await _deliver( + helper, + _TxtPacket("frobnicate", 1, sender_timestamp=100), + identity_type="room_server", + name="room", + ) + + room.cli.handle_command.assert_called_once() + room.add_post.assert_not_awaited() + + @pytest.mark.asyncio async def test_repeater_runs_any_accepted_type_without_a_text_test(): """[fails pre-fix] A repeater has no chat, so every accepted type is a command. @@ -659,7 +687,12 @@ async def test_repeater_command_is_not_run_twice(second_timestamp, label): @pytest.mark.asyncio async def test_repeater_command_runs_again_for_a_newer_timestamp(): - """The guard blocks replays, not legitimate repeat commands.""" + """A guard against over-blocking: a newer timestamp is fresh work. + + Prerequisite rather than regression proof -- it passes with the guard + removed too. It is here so a future tightening of _accept_once cannot + quietly reject the legitimate case. + """ helper = _repeater_cli_helper() await _deliver(helper, _TxtPacket("reboot", 1, sender_timestamp=100)) @@ -760,11 +793,74 @@ def test_may_ack_stays_quiet_for_an_unknown_client(): def test_may_ack_keeps_acking_when_the_core_reports_no_type(): - """An older core publishes no txt_type; do not go silent on that account.""" + """An older core publishes no txt_type; do not go silent on that account. + + Reached only through the compatibility fallback: a core old enough to omit + the type is also too old to install the policy, so this pins _may_ack's own + contract rather than a path the pair can take together. + """ client = _FakeClient(pubkey=bytes([0x21]) + b"x" * 31, shared_secret=b"k" * 32, permissions=0) assert _may_ack("room_server", client, None, 100) is True +@pytest.mark.asyncio +@pytest.mark.parametrize( + "permissions,expect_ack", + [ + (PERM_ACL_READ_WRITE, True), # a writer's post is stored and answered + (0, False), # PERM_ACL_GUEST -- neither stored nor answered + ], +) +async def test_guest_plain_post_earns_no_ack_on_the_wire(permissions, expect_ack): + """[fails pre-fix] End to end: a guest's post produces nothing on the air. + + This drives the whole MC-R4 seam with real crypto -- a genuinely encrypted + PLAIN DM, a real registered room identity, core's real TextMessageHandler, + the real ACL -- and watches the injector. Core schedules the ACK before + this helper's gates run, so without the policy the guest would see delivery + confirmed for a post that was then refused. + + simple_room_server::onPeerDataRecv gives a guest neither storage nor ACK. + """ + if not CORE_ACCEPTS_ACK_POLICY: + pytest.skip("installed openhop_core has no should_ack_fn hook") + + server, sender = _distinct_identities(2) + acl = ACL() + client = ClientInfo(Identity(sender.get_public_key()), permissions=permissions) + client.shared_secret = Identity(sender.get_public_key()).calc_shared_secret( + server.get_private_key() + ) + acl.clients[sender.get_public_key()] = client + + injector = AsyncMock(return_value=True) + helper = TextHelper( + identity_manager=MagicMock(), + packet_injector=injector, + acl_dict={server.get_public_key()[0]: acl}, + ) + room = MagicMock() + room.add_post = AsyncMock(return_value=True) + room.cli = None + helper.register_identity("room", server, identity_type="room_server", radio_config={}) + helper.room_servers[server.get_public_key()[0]] = room + + packet, _crc = PacketBuilder.create_text_message( + _SendDest(server.get_public_key()), sender, "hello room", 0, "direct", None, 0 + ) + assert await helper.process_text_packet(packet) is True + + # The ACK is scheduled on a delay; give it room to fire if it was going to. + await asyncio.sleep(TXT_ACK_DELAY_MS / 1000.0 + 0.2) + + acked = any( + getattr(call.args[0], "get_payload_type", lambda: None)() == PAYLOAD_TYPE_ACK + for call in injector.await_args_list + ) + assert acked is expect_ack + assert room.add_post.await_count == (1 if expect_ack else 0) + + def test_register_identity_survives_a_core_without_the_ack_hook(): """[fails pre-fix] An older core must not take the node off the air. @@ -811,7 +907,11 @@ def test_register_identity_hands_the_core_handler_an_ack_policy(): ) identity = _FakeId(bytes([0x33]) + b"x" * 31) + # Pin the capability flag: this test is about what the policy decides, not + # about which core happens to be importable, and indexing the kwarg would + # KeyError under an older one. with ( + patch("repeater.handler_helpers.text.CORE_ACCEPTS_ACK_POLICY", True), patch("repeater.handler_helpers.text.TextMessageHandler") as tmh, patch("repeater.handler_helpers.text.MeshCLI", return_value=MagicMock()), ): diff --git a/tests/test_handler_helpers_room_server.py b/tests/test_handler_helpers_room_server.py index 7f02a76..733faad 100644 --- a/tests/test_handler_helpers_room_server.py +++ b/tests/test_handler_helpers_room_server.py @@ -367,6 +367,49 @@ async def test_room_server_push_post_to_client_clamps_oversized_legacy_stored_te assert text_portion.decode("utf-8") == "q" * MAX_POST_TEXT_LEN +@pytest.mark.asyncio +async def test_room_server_push_ends_the_text_at_an_embedded_nul(): + """[fails pre-fix] A stored post with an interior NUL is cut where firmware cuts it. + + pushPostToClient sizes the body with `strlen(post.text)`, for the payload + and the expected ACK alike. The web API will store "a\0b", so pushing it + whole sends text the receiver cannot see -- it stops at the NUL -- and, + worse, hashes the pending ACK over a span the receiver never reproduces. + The push would then never be acknowledged and would retry into the + failure backoff. + """ + db = _FakeDB() + db.get_client_sync.return_value = {"push_failures": 0} + injector = AsyncMock(return_value=True) + rs = _make_room_server(db=db, injector=injector) + rs.global_limiter = SimpleNamespace(acquire=AsyncMock(), release=MagicMock()) + rs._handle_ack_received = AsyncMock() + + client = _FakeClient(pubkey=b"E" * 32, out_path=b"\xaa\xbb", out_path_len=2) + post = { + "author_pubkey": (b"F" * 32).hex(), + "message_text": "a\x00b", + "post_timestamp": 1234.5, + } + + packet = SimpleNamespace(path=bytearray(), path_len=0) + with ( + patch( + "repeater.handler_helpers.room_server.CryptoUtils.sha256", + return_value=b"\x01\x02\x03\x04abcd", + ), + patch( + "repeater.handler_helpers.room_server.PacketBuilder.create_datagram", + return_value=packet, + ) as create_datagram, + ): + ok = await rs.push_post_to_client(client, post) + + assert ok is True + # Prefix is timestamp(4) + flags(1) + author_prefix(4) = 9 bytes. + assert create_datagram.call_args.kwargs["plaintext"][9:] == b"a" + + @pytest.mark.asyncio async def test_room_server_push_expected_ack_matches_firmware_signed_ack(): """The pending ACK CRC must match what a signed-plain receiver sends back.