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",