From 0c1aa4fd15cdcab44a41b2e4c07b65be6c417ae7 Mon Sep 17 00:00:00 2001 From: agessaman Date: Tue, 15 Sep 2026 22:39:34 -0700 Subject: [PATCH] fix(region): never warn the synthetic "Channel User" sender A channel message with no "Name: " prefix falls back to a stand-in name. That is not a node: every such message shares the identity, so they would accumulate a run together and then get a DM addressed to a contact that does not exist. The hook now passes no sender for those, so they are still counted but can never earn a warning. The literal is a named constant and a test pins the hook's guard to it. Also folds the channel body-budget formula into models.channel_body_limit rather than letting the web viewer keep a fourth copy of "max(130, 160 - len(name) - 2)"; BaseCommand and CommandManager now call it too, and 158 becomes models.DM_BODY_LIMIT. --- docs/region-warnings.md | 2 + modules/command_manager.py | 11 +++-- modules/commands/base_command.py | 13 +++-- modules/message_handler.py | 10 +++- modules/models.py | 18 +++++++ modules/region_warning.py | 15 ++++-- modules/web_viewer/app.py | 13 +++-- tests/unit/test_region_warning.py | 82 ++++++++++++++++++++++++++++++- 8 files changed, 146 insertions(+), 18 deletions(-) diff --git a/docs/region-warnings.md b/docs/region-warnings.md index 80defd5..92429a5 100644 --- a/docs/region-warnings.md +++ b/docs/region-warnings.md @@ -27,6 +27,8 @@ Only `global` can earn a warning, and only on positive evidence. If the radio di `unknown` is excluded from the unscoped percentage rather than counted as clean, so a mesh the bot cannot classify reads as "no data" instead of "no problem". +A channel message that arrives with no `Name: ` prefix has no attributable sender. It is still counted, but it can never earn anyone a warning — every such message would otherwise share one synthetic identity. + Messages the radio cached from before the current connection are skipped entirely — a reconnect replays them as a burst, and counting them would both distort the tallies and let stale traffic earn someone a warning. ## Reading the page diff --git a/modules/command_manager.py b/modules/command_manager.py index 2233708..5898c95 100644 --- a/modules/command_manager.py +++ b/modules/command_manager.py @@ -30,7 +30,12 @@ from .config_validation import ( _channel_name_is_public, strip_optional_quotes, ) -from .models import CHANNEL_REGIONAL_FLOOD_SCOPE_BODY_OVERHEAD, MeshMessage +from .models import ( + CHANNEL_REGIONAL_FLOOD_SCOPE_BODY_OVERHEAD, + DM_BODY_LIMIT, + MeshMessage, + channel_body_limit, +) from .plugin_loader import PluginLoader from .security_utils import sanitize_name, validate_safe_path from .utils import check_internet_connectivity_async, decode_escape_sequences, format_keyword_response_with_placeholders @@ -687,7 +692,7 @@ class CommandManager: can be called outside of a specific command instance. """ if message.is_dm: - return 158 + return DM_BODY_LIMIT username: str | None = None try: if hasattr(self.bot, 'meshcore') and self.bot.meshcore: @@ -701,7 +706,7 @@ class CommandManager: pass if not username: username = self.bot.config.get('Bot', 'bot_name', fallback='Bot') - max_length = max(130, 160 - len(str(username).encode('utf-8')) - 2) + max_length = channel_body_limit(username) if not MeshMessage.is_global_flood_scope(message.effective_outgoing_flood_scope(self.bot)): max_length -= CHANNEL_REGIONAL_FLOOD_SCOPE_BODY_OVERHEAD return max_length diff --git a/modules/commands/base_command.py b/modules/commands/base_command.py index 76c34c7..c2bdc48 100644 --- a/modules/commands/base_command.py +++ b/modules/commands/base_command.py @@ -19,7 +19,12 @@ from ..command_prefix import ( normalize_command_content, ) from ..config_schema import LEGACY_ENABLED_ALIASES -from ..models import CHANNEL_REGIONAL_FLOOD_SCOPE_BODY_OVERHEAD, MeshMessage +from ..models import ( + CHANNEL_REGIONAL_FLOOD_SCOPE_BODY_OVERHEAD, + DM_BODY_LIMIT, + MeshMessage, + channel_body_limit, +) from ..security_utils import validate_pubkey_format from ..utils import ( format_elapsed_display, @@ -689,7 +694,7 @@ class BaseCommand(ABC): int: Maximum message body length in UTF-8 bytes. """ if message.is_dm: - return 158 + return DM_BODY_LIMIT # For channel messages, calculate based on bot username length # Try to get device username from meshcore first (actual radio username) @@ -712,9 +717,7 @@ class BaseCommand(ABC): if not username: username = self.bot.config.get('Bot', 'bot_name', fallback='Bot') - # 160 bytes are available for channel messages - # Calculate max length: 160 - username_length - 2 (for ": ") - max_length = max(130, 160 - len(str(username).encode('utf-8')) - 2) + max_length = channel_body_limit(username) if not MeshMessage.is_global_flood_scope(message.effective_outgoing_flood_scope(self.bot)): max_length -= CHANNEL_REGIONAL_FLOOD_SCOPE_BODY_OVERHEAD return max_length diff --git a/modules/message_handler.py b/modules/message_handler.py index a8471d8..b8de8f1 100644 --- a/modules/message_handler.py +++ b/modules/message_handler.py @@ -30,6 +30,10 @@ from .utils import ( # MessageHandler.find_recent_rf_data. Anything other than a fallback is known to be # this message's own packet; a fallback is merely the most recent packet heard, so its # route belongs to some other transmission and must not be attributed (issue #80). +# Stand-in used when a channel message carries no "Name: " prefix to extract a +# sender from. It is not a node: every such message would share this identity. +CHANNEL_SENDER_FALLBACK = "Channel User" + RF_MATCH_KEY = "_rf_match" RF_MATCH_EXACT = "exact" RF_MATCH_PUBKEY = "pubkey" @@ -2608,7 +2612,7 @@ class MessageHandler: # Get sender information from text field if it's in "SENDER: message" format text = payload.get("text", "") - sender_id = "Channel User" # Default fallback + sender_id = CHANNEL_SENDER_FALLBACK # Default fallback # Try to extract sender from text field (e.g., "HOWL: Test" -> "HOWL") message_content = text # Default to full text @@ -2837,7 +2841,9 @@ class MessageHandler: # allowlist drops — running it after the gate would blind the # monitor to the traffic it exists to measure. await self._observe_flood_scope( - sender_id=sender_id, + # A message with no "Name: " prefix has no attributable sender, + # so it is counted but can never earn anyone a warning. + sender_id=None if sender_id == CHANNEL_SENDER_FALLBACK else sender_id, sender_pubkey=payload.get("pubkey_prefix", ""), channel=channel_name, sender_timestamp=payload.get("sender_timestamp", 0), diff --git a/modules/models.py b/modules/models.py index 99c8fb5..cb7c85a 100644 --- a/modules/models.py +++ b/modules/models.py @@ -10,6 +10,24 @@ from typing import Any, Optional # Firmware reserves extra bytes for regional (non-global) TC_FLOOD scope on channel text. CHANNEL_REGIONAL_FLOOD_SCOPE_BODY_OVERHEAD = 10 +# A DM carries no username prefix, so the whole cipher block is body. +DM_BODY_LIMIT = 158 + + +def channel_body_limit(username: Optional[str]) -> int: + """Global-scope body budget in UTF-8 bytes for a channel message from ``username``. + + Channel messages go out as ``": "``, so the budget is the + 160-byte cipher block minus the name and the ``": "``. Regional scope costs + a further ``CHANNEL_REGIONAL_FLOOD_SCOPE_BODY_OVERHEAD``, which callers + subtract themselves once they know the outgoing scope. + + Shared by the command layer and the web viewer, which computes the same + number in a process that has no bot object. + """ + name = str(username or "Bot") + return max(130, 160 - len(name.encode("utf-8")) - 2) + @dataclass class MeshMessage: diff --git a/modules/region_warning.py b/modules/region_warning.py index 47d24cc..7d0009e 100644 --- a/modules/region_warning.py +++ b/modules/region_warning.py @@ -38,6 +38,9 @@ from dataclasses import dataclass, field from datetime import date, datetime, timedelta from typing import Any, Optional +from modules.models import DM_BODY_LIMIT as _DM_BODY_LIMIT +from modules.models import channel_body_limit + CONFIG_SECTION = "Region_Warnings" # Verdicts produced by MessageHandler._classify_channel_flood_scope. @@ -64,8 +67,9 @@ DEFAULT_MESSAGE = ( "Setting one in your MeshCore app keeps things quiet. Thanks!" ) -# Body budget for a DM, matching CommandManager.get_max_message_length. -DM_BODY_LIMIT = 158 +# Re-exported so the web viewer (which has no bot object) can size the preview +# against the same budget the command layer enforces. +DM_BODY_LIMIT = _DM_BODY_LIMIT @dataclass(frozen=True) @@ -577,14 +581,17 @@ class RegionWarningMonitor: return ok, body if ok else f"DM send failed (contact unknown or radio busy): {body}" def _channel_body_limit(self) -> int: - """Channel body budget for a global-scope send from this node.""" + """Channel body budget for a global-scope send from this node. + + Warnings always go out unscoped, so no regional-scope overhead applies. + """ try: from modules.models import MeshMessage probe = MeshMessage(content="", channel="", is_dm=False, reply_scope="") return int(self.bot.command_manager.get_max_message_length(probe)) except Exception: - return 130 + return channel_body_limit(None) # -- gating ------------------------------------------------------------ diff --git a/modules/web_viewer/app.py b/modules/web_viewer/app.py index 084dc76..2a8ea9d 100644 --- a/modules/web_viewer/app.py +++ b/modules/web_viewer/app.py @@ -56,6 +56,7 @@ from modules.db_retention import ( ) from modules.ini_writer import IniValueError, update_ini_values from modules.maintenance import MaintenanceRunner +from modules.models import channel_body_limit from modules.scheduled_message_admin import ( SECTION as SCHEDULED_MESSAGES_SECTION, ) @@ -4051,9 +4052,15 @@ class BotDataViewer: # ── Region warnings (regional flood scope) ─────────────────────────── def _region_warning_channel_limit() -> int: - """Channel body budget for a global-scope send, mirroring CommandManager.""" - name = (self.config.get('Bot', 'bot_name', fallback='Bot') or 'Bot').strip() or 'Bot' - return max(130, 160 - len(name.encode('utf-8')) - 2) + """Channel body budget for a global-scope send. + + The device's own name is authoritative for the command layer, but + the viewer is a separate process with no radio, so it falls back to + the configured one. They match on any install where the bot manages + the device name. + """ + name = (self.config.get('Bot', 'bot_name', fallback='Bot') or 'Bot').strip() + return channel_body_limit(name or 'Bot') @self.app.route('/api/region-warnings') def api_region_warnings(): diff --git a/tests/unit/test_region_warning.py b/tests/unit/test_region_warning.py index 2317528..eaaf6c8 100644 --- a/tests/unit/test_region_warning.py +++ b/tests/unit/test_region_warning.py @@ -11,7 +11,11 @@ import pytest from modules import region_warning from modules.db_migrations import MigrationRunner from modules.enums import PayloadType, RouteType -from modules.message_handler import RF_MATCH_KEY, MessageHandler +from modules.message_handler import ( + CHANNEL_SENDER_FALLBACK, + RF_MATCH_KEY, + MessageHandler, +) from modules.region_warning import ( ACTION_DRY_RUN, ACTION_FAILED, @@ -163,6 +167,71 @@ class TestClassifyFloodScope: ) == VERDICT_SCOPED +@pytest.mark.asyncio +class TestObservationHook: + """The MessageHandler side of the hook, independent of the monitor.""" + + def _hooked_handler(self): + handler = _handler() + handler.bot.region_warning_monitor = MagicMock() + handler.bot.region_warning_monitor.observe = AsyncMock() + handler.bot.connection_time = None + return handler + + async def _observe(self, handler, **overrides): + args = { + "sender_id": "Ann", + "sender_pubkey": "ab", + "channel": "#gen", + "sender_timestamp": 0, + "reply_scope": None, + "recent_rf_data": None, + "packet_info": None, + "scope_rf_data": None, + "scope_packet_info": None, + } + args.update(overrides) + await handler._observe_flood_scope(**args) + + async def test_verdict_is_forwarded_to_the_monitor(self): + handler = self._hooked_handler() + await self._observe( + handler, recent_rf_data=_rf(route_type_int=int(RouteType.FLOOD.value))) + kwargs = handler.bot.region_warning_monitor.observe.await_args.kwargs + assert kwargs["verdict"] == VERDICT_GLOBAL + assert kwargs["sender_id"] == "Ann" + + async def test_no_monitor_is_a_noop(self): + handler = self._hooked_handler() + handler.bot.region_warning_monitor = None + await self._observe(handler) + + async def test_old_cached_message_is_skipped(self): + """A reconnect replays cached traffic; counting it would distort everything.""" + handler = self._hooked_handler() + handler.bot.connection_time = 2_000 + await self._observe(handler, sender_timestamp=1_000) + handler.bot.region_warning_monitor.observe.assert_not_awaited() + + async def test_monitor_failure_does_not_escape_to_the_message_path(self): + handler = self._hooked_handler() + handler.bot.region_warning_monitor.observe = AsyncMock( + side_effect=RuntimeError("boom")) + await self._observe(handler) + handler.logger.exception.assert_called() + + async def test_unattributable_sender_is_passed_through_as_none(self): + handler = self._hooked_handler() + await self._observe(handler, sender_id=None) + kwargs = handler.bot.region_warning_monitor.observe.await_args.kwargs + assert kwargs["sender_id"] is None + + +def test_channel_sender_fallback_is_the_handler_literal(): + """The hook drops this name; if the handler renames it, the guard silently dies.""" + assert CHANNEL_SENDER_FALLBACK == "Channel User" + + # --------------------------------------------------------------------------- # Settings # --------------------------------------------------------------------------- @@ -269,6 +338,17 @@ class TestObservation: verdict=VERDICT_UNKNOWN, sender_id="Ann", sender_pubkey="ab", channel="#gen") monitor.bot.command_manager.send_dm.assert_not_called() + async def test_unattributable_sender_is_counted_but_never_warned(self): + """A channel message with no "Name: " prefix has no real sender.""" + monitor = _monitor(enabled="true", dry_run="false", min_unscoped_messages=1) + for _ in range(5): + await monitor.observe( + verdict=VERDICT_GLOBAL, sender_id=None, sender_pubkey="", channel="#gen") + monitor.bot.command_manager.send_dm.assert_not_called() + rows = monitor.bot.db_manager.execute_query( + "SELECT global_count FROM region_scope_daily") + assert rows[0]["global_count"] == 5 + async def test_bookkeeping_failure_does_not_raise(self): monitor = _monitor() monitor.bot.db_manager = None