From 0d50f5feec104ac72282ab800a53eb304342baab Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 4 Sep 2026 22:06:52 -0700 Subject: [PATCH] fix(text): dispatch on the text type and guard replays Two things firmware does that openHop did not. Dispatch by type, not by text. simple_room_server::onPeerDataRecv routes CLI_DATA/CLI_COMMAND to the admin CLI and turns PLAIN into a post; simple_repeater has no chat function, so it runs every accepted type through handleCommand with no text test at all. openHop asked _is_cli_command(message_text) for both, and so got both wrong: a PLAIN "set ..." ran as a command instead of being posted, a command whose text was not in the prefix list was published to the room, and an admin's mistyped repeater command vanished silently instead of being answered. Guard replays. Both servers open the branch with `sender_timestamp >= client->last_timestamp`, treat equality as a retry they accept but do not re-run, and drop anything older. openHop checked neither, so replaying an admin command with the same timestamp and different attempt bits re-executed it -- packet dedup does not catch that because the bytes differ. Note ACL._is_replay is not reusable here: its strict <= rejects the equal case this branch accepts. Both read from packet.decrypted, which openhop_core now publishes. A core too old to report them falls back to the previous text-prefix behaviour and logs a warning rather than dropping every message. --- repeater/handler_helpers/text.py | 100 ++++++++++- ...test_handler_helpers_path_protocol_text.py | 169 +++++++++++++++--- 2 files changed, 234 insertions(+), 35 deletions(-) diff --git a/repeater/handler_helpers/text.py b/repeater/handler_helpers/text.py index 0c53b36..6991b72 100644 --- a/repeater/handler_helpers/text.py +++ b/repeater/handler_helpers/text.py @@ -41,6 +41,36 @@ TXT_TYPE_CLI_COMMAND = 0x03 SERVER_TXT_TYPES = frozenset((TXT_TYPE_PLAIN, TXT_TYPE_CLI_DATA, TXT_TYPE_CLI_COMMAND)) +def _accept_once(client, sender_timestamp) -> tuple[bool, bool]: + """Firmware's TXT_MSG replay guard. Returns ``(accepted, is_retry)``. + + ``simple_repeater`` and ``simple_room_server`` both open the branch with + ``sender_timestamp >= client->last_timestamp``: an older timestamp is a + replay and is dropped whole, an equal one is a *retry* — accepted, activity + refreshed, but the command not re-run and the post not re-added — and a + newer one is fresh work. The watermark advances either way. + + Note the deliberate difference from ``ACL._is_replay``, which rejects the + equal case: that matches the REQ branch (``timestamp > last_timestamp``), + not this one. + + An unresolved client or a core too old to publish the timestamp yields + ``(True, False)`` — no watermark to check against, so behave as before. + """ + if client is None or sender_timestamp is None: + return True, False + last = int(getattr(client, "last_timestamp", 0) or 0) + if sender_timestamp < last: + logger.warning( + f"Possible replay: text timestamp={sender_timestamp} < last={last}; dropping" + ) + return False, False + is_retry = sender_timestamp == last + client.last_timestamp = sender_timestamp + client.last_activity = int(time.time()) + return True, is_retry + + def _is_guest_client(client) -> bool: """True when ``client`` holds the GUEST role (firmware ``PERM_ACL_GUEST``). @@ -323,11 +353,12 @@ class TextHelper: # A core older than the one that publishes ``txt_type`` reports # None; behave as before rather than dropping every message. txt_type = packet.decrypted.get("txt_type") - if txt_type is None: + sender_timestamp = packet.decrypted.get("sender_timestamp") + if txt_type is None or sender_timestamp is None: logger.warning( - "TXT_MSG carried no txt_type: openhop_core is older than the " - "build that publishes it, so the text-type gate is open and a " - "signed post can reach the CLI. Upgrade openhop_core." + "TXT_MSG carried no txt_type/sender_timestamp: openhop_core is " + "older than the build that publishes them, so the text-type " + "gate and the replay guard are open. Upgrade openhop_core." ) if txt_type is not None and txt_type not in SERVER_TXT_TYPES: logger.debug( @@ -338,6 +369,15 @@ class TextHelper: sender_client = self._resolve_sender_client(dest_hash, src_hash, packet) + # Firmware checks the type, then the replay watermark, and only then + # looks at the message. A retry is accepted but re-runs nothing. + accepted, is_retry = _accept_once(sender_client, sender_timestamp) + if not accepted: + logger.warning( + f"[{identity_type}:{identity_name}] replay from 0x{src_hash:02X}; dropping" + ) + return + # Clean message text - remove null bytes and trailing whitespace message_text = message_text.rstrip("\x00").rstrip() @@ -347,8 +387,19 @@ class TextHelper: if identity_type == "room_server" and dest_hash in self.room_servers: room_server = self.room_servers[dest_hash] - # Check if this is a CLI command FIRST (before storing as post) - if self._is_cli_command(message_text): + # simple_room_server::onPeerDataRecv splits by *type*, not by + # text: CLI_DATA/CLI_COMMAND go to the admin CLI, PLAIN becomes + # a post. Deciding from the text instead got both wrong -- a + # PLAIN "set ..." ran as a command instead of being posted, and + # a command whose text was not in the prefix list was published + # to the room. + if txt_type is None: + # Older core: no type to dispatch on, keep the text test. + room_is_command = self._is_cli_command(message_text) + else: + room_is_command = txt_type in (TXT_TYPE_CLI_DATA, TXT_TYPE_CLI_COMMAND) + + if room_is_command: # Handle CLI command - do NOT store as post if room_server and room_server.cli: try: @@ -368,6 +419,16 @@ class TextHelper: if sender_client is not None: sender_pubkey = sender_client.id.get_public_key() + # A retry re-sends the same command; firmware + # answers it with an empty reply rather than running + # it a second time. + if is_retry: + logger.info( + f"Room '{identity_name}': retry of command from " + f"0x{src_hash:02X}; not re-running" + ) + return + # Handle CLI command reply = room_server.cli.handle_command( sender_pubkey=sender_pubkey, command=message_text, is_admin=is_admin @@ -409,6 +470,15 @@ class TextHelper: ) return + # Firmware calls addPost only when `!is_retry`, so a resent post + # does not appear in the room twice. + if is_retry: + logger.info( + f"Room '{identity_name}': retry of post from 0x{src_hash:02X}; " + "not storing again" + ) + return + try: # Get sender's full pubkey sender_pubkey = bytes([src_hash]) + b"\x00" * 31 # Default @@ -434,8 +504,16 @@ class TextHelper: return - # Check if this is a CLI command to the repeater (AFTER decryption) - if dest_hash == self.repeater_hash and self.cli and self._is_cli_command(message_text): + # A repeater has no chat function, so simple_repeater::onPeerDataRecv + # runs *every* accepted type from an admin through handleCommand with + # no text test at all. Matching that means a plain DM to the repeater + # is a command too -- which it always was; openHop simply dropped the + # ones whose text did not match a prefix. + if txt_type is None and not self._is_cli_command(message_text): + # Older core: no type to dispatch on, keep the text test. + return + + if dest_hash == self.repeater_hash and self.cli: try: repeater_hash = self.repeater_hash if repeater_hash is None: @@ -458,6 +536,12 @@ class TextHelper: if sender_client is not None: sender_pubkey = sender_client.id.get_public_key() + # A retry re-sends the same command; firmware answers it + # with an empty reply rather than running it again. + if is_retry: + logger.info(f"Retry of command from 0x{src_hash:02X}; not re-running") + return + # Handle CLI command reply = self.cli.handle_command( sender_pubkey=sender_pubkey, command=message_text, is_admin=is_admin diff --git a/tests/test_handler_helpers_path_protocol_text.py b/tests/test_handler_helpers_path_protocol_text.py index f6de021..03db3ad 100644 --- a/tests/test_handler_helpers_path_protocol_text.py +++ b/tests/test_handler_helpers_path_protocol_text.py @@ -459,16 +459,53 @@ def test_text_helper_cli_prefix_and_admin_permission_checks(): class _TxtPacket: - """A packet as the core TextMessageHandler leaves it: text + its type.""" + """A packet as the core TextMessageHandler leaves it. - def __init__(self, text: str, txt_type=None): + ``txt_type``/``sender_timestamp`` default to absent so a test can model a + core too old to publish them. + """ + + def __init__(self, text: str, txt_type=None, sender_timestamp=None): self.decrypted = {"text": text} if txt_type is not None: self.decrypted["txt_type"] = txt_type + if sender_timestamp is not None: + self.decrypted["sender_timestamp"] = sender_timestamp self.payload = bytearray([0x41, 0x21]) self.mark_do_not_retransmit = MagicMock() +def _room_helper(permissions=PERM_ACL_ADMIN): + """A TextHelper wired as a room-server identity with a stub CLI and store.""" + helper = TextHelper(identity_manager=MagicMock(), acl_dict={}) + room = MagicMock() + room.add_post = AsyncMock(return_value=True) + room.cli = MagicMock() + room.cli.handle_command = MagicMock(return_value="ok") + helper.room_servers = {0x41: room} + helper.handlers = {0x41: {"name": "room", "type": "room_server", "identity": MagicMock()}} + helper._resolve_sender_client = MagicMock( + return_value=_FakeClient( + pubkey=bytes([0x21]) + b"x" * 31, + shared_secret=b"k" * 32, + permissions=permissions, + ) + ) + helper._check_admin_permission_for_identity = MagicMock(return_value=True) + helper._send_cli_reply = AsyncMock() + return helper, room + + +async def _deliver(helper, packet, identity_type="repeater", name="rep"): + await helper._on_message_received( + identity_name=name, + identity_type=identity_type, + packet=packet, + dest_hash=0x41, + src_hash=0x21, + ) + + def _repeater_cli_helper(): """A TextHelper wired as a repeater identity whose admin CLI is a stub.""" helper = TextHelper(identity_manager=MagicMock(), acl_dict={}) @@ -500,7 +537,7 @@ def _repeater_cli_helper(): ], ) async def test_text_helper_runs_cli_only_for_server_text_types(txt_type, runs): - """[fails pre-fix for SIGNED_PLAIN] The text type gates the CLI, not the text. + """[fails pre-fix for SIGNED_PLAIN] Only the accepted types reach the CLI. simple_repeater::onPeerDataRecv opens its TXT_MSG branch by rejecting any flags outside {PLAIN, CLI_DATA, CLI_COMMAND}. openHop dispatched purely on @@ -509,6 +546,9 @@ async def test_text_helper_runs_cli_only_for_server_text_types(txt_type, runs): bare text -- ran as an admin CLI command whenever it happened to start with a command prefix. + All three accepted types run here because a repeater has no chat function; + that a *plain* one runs with no text test is covered separately. + ``txt_type is None`` is a core too old to report it; that must keep working rather than silently dropping every message. """ @@ -534,36 +574,111 @@ async def test_text_helper_signed_plain_is_not_stored_as_a_room_post(): simple_room_server::onPeerDataRecv carries the same filter as the repeater; only PLAIN becomes a post there. openHop stored whatever arrived. """ - helper = TextHelper(identity_manager=MagicMock(), acl_dict={}) - room = MagicMock() - room.add_post = AsyncMock(return_value=True) - room.cli = None - helper.room_servers = {0x41: room} - helper.handlers = {0x41: {"name": "room", "type": "room_server", "identity": MagicMock()}} - helper._resolve_sender_client = MagicMock( - return_value=_FakeClient(pubkey=bytes([0x21]) + b"x" * 31, shared_secret=b"k" * 32) - ) + helper, room = _room_helper(permissions=PERM_ACL_READ_WRITE) - await helper._on_message_received( - identity_name="room", - identity_type="room_server", - packet=_TxtPacket("hello room", 2), - dest_hash=0x41, - src_hash=0x21, - ) + await _deliver(helper, _TxtPacket("hello room", 2, sender_timestamp=100), "room_server", "room") room.add_post.assert_not_awaited() - # ...while a plain post still lands. - await helper._on_message_received( - identity_name="room", - identity_type="room_server", - packet=_TxtPacket("hello room", 0), - dest_hash=0x41, - src_hash=0x21, - ) + # ...while a plain post from the same writer still lands. + await _deliver(helper, _TxtPacket("hello room", 0, sender_timestamp=101), "room_server", "room") room.add_post.assert_awaited_once() +@pytest.mark.asyncio +@pytest.mark.parametrize( + "txt_type,is_command", + [ + (0, False), # PLAIN -> a post + (1, True), # CLI_DATA -> the admin CLI + (3, True), # CLI_COMMAND -> the admin CLI + ], +) +async def test_room_dispatches_on_type_not_text(txt_type, is_command): + """[fails pre-fix] The room splits command from post by type, as firmware does. + + simple_room_server::onPeerDataRecv routes CLI_DATA/CLI_COMMAND through + handleCommand and turns PLAIN into a post. openHop asked + `_is_cli_command(message_text)` instead, so it got both wrong: a PLAIN + "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. + """ + helper, room = _room_helper() + + await _deliver( + helper, + _TxtPacket("get status", txt_type, sender_timestamp=100), + identity_type="room_server", + name="room", + ) + + assert room.cli.handle_command.called is is_command + assert room.add_post.await_count == (0 if is_command else 1) + + +@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. + + simple_repeater::onPeerDataRecv hands PLAIN, CLI_DATA and CLI_COMMAND alike + to handleCommand with no text test. openHop dropped anything whose text did + not start with a known prefix, so an admin's mistyped command vanished + silently instead of being answered. + """ + helper = _repeater_cli_helper() + + await _deliver(helper, _TxtPacket("frobnicate", 1, sender_timestamp=100)) + + helper.cli.handle_command.assert_called_once() + assert helper.cli.handle_command.call_args.kwargs["command"] == "frobnicate" + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "second_timestamp,label", + [(99, "older"), (100, "equal")], +) +async def test_repeater_command_is_not_run_twice(second_timestamp, label): + """[fails pre-fix] A resent command is not executed again. + + simple_repeater guards on `sender_timestamp >= client->last_timestamp` and + treats equality as a retry it answers with an empty reply; an older stamp is + dropped whole. openHop checked neither, so replaying an admin command with + the same timestamp and different attempt bits re-ran it -- packet dedup does + not catch that, because the bytes differ. + """ + helper = _repeater_cli_helper() + + await _deliver(helper, _TxtPacket("reboot", 1, sender_timestamp=100)) + assert helper.cli.handle_command.call_count == 1 + + await _deliver(helper, _TxtPacket("reboot", 1, sender_timestamp=second_timestamp)) + assert helper.cli.handle_command.call_count == 1, f"{label} timestamp re-ran the command" + + +@pytest.mark.asyncio +async def test_repeater_command_runs_again_for_a_newer_timestamp(): + """The guard blocks replays, not legitimate repeat commands.""" + helper = _repeater_cli_helper() + + await _deliver(helper, _TxtPacket("reboot", 1, sender_timestamp=100)) + await _deliver(helper, _TxtPacket("reboot", 1, sender_timestamp=101)) + + assert helper.cli.handle_command.call_count == 2 + + +@pytest.mark.asyncio +async def test_room_post_is_not_stored_twice_for_a_retry(): + """[fails pre-fix] firmware calls addPost only when `!is_retry`.""" + helper, room = _room_helper(permissions=PERM_ACL_READ_WRITE) + + await _deliver(helper, _TxtPacket("hello", 0, sender_timestamp=100), "room_server", "room") + await _deliver(helper, _TxtPacket("hello", 0, sender_timestamp=100), "room_server", "room") + + assert room.add_post.await_count == 1 + + @pytest.mark.asyncio @pytest.mark.parametrize( "permissions,posts",