diff --git a/modules/commands/path_command.py b/modules/commands/path_command.py index 8b60e94..e4a3a34 100644 --- a/modules/commands/path_command.py +++ b/modules/commands/path_command.py @@ -382,13 +382,31 @@ class PathCommand(BaseCommand): bph = self._bytes_per_hop_from_nodes_and_routing(node_ids, routing_info) return bph >= self.minimum_path_bytes - def _format_path_distance(self) -> str: + def _store_path_distance( + self, distance_km: Optional[float], message: Optional[MeshMessage] + ) -> None: + """Record the distance for this request. + + Attached to the message when we have it, because the decode above awaits a + database lookup and a second path command can interleave there. Instance + state would let one request render the other's distance. The attribute + fallback keeps direct calls (and existing tests) working. + """ + if message is not None: + message._path_distance_km = distance_km # type: ignore[attr-defined] + self._last_path_distance_km = distance_km + + def _format_path_distance(self, message: Optional[MeshMessage] = None) -> str: """Render the {path_distance} placeholder; empty when the path cannot be measured. Matches the ``{path_distance}`` name and ``12.4km`` shape already used by the test command, so one prefix template reads the same across both commands. """ - distance = getattr(self, '_last_path_distance_km', None) + distance = None + if message is not None: + distance = getattr(message, '_path_distance_km', None) + if distance is None: + distance = getattr(self, '_last_path_distance_km', None) if distance is None: return '' return f"{distance:.1f}km" @@ -397,7 +415,7 @@ class PathCommand(BaseCommand): if not self.path_reply_prefix: return '' fields = self.get_standard_placeholder_fields(message) - fields['path_distance'] = self._format_path_distance() + fields['path_distance'] = self._format_path_distance(message) formatted = format_piped_template( self.path_reply_prefix, {k: str(v) for k, v in fields.items()}, @@ -418,14 +436,19 @@ class PathCommand(BaseCommand): ) async def _decode_node_ids( - self, node_ids: list[str], routing_info: Optional[dict[str, Any]] = None + self, + node_ids: list[str], + routing_info: Optional[dict[str, Any]] = None, + message: Optional[MeshMessage] = None, ) -> str: self.logger.info(f"Decoding path with {len(node_ids)} nodes: {','.join(node_ids)}") if not self._should_resolve_repeater_names(node_ids, routing_info): - self._last_path_distance_km = None + self._store_path_distance(None, message) return self._format_repeater_resolution_deferred(node_ids) repeater_info = await self._lookup_repeater_names(node_ids) - self._last_path_distance_km = self._calculate_path_distance_km(node_ids, repeater_info) + self._store_path_distance( + self._calculate_path_distance_km(node_ids, repeater_info), message + ) return self._format_path_response(node_ids, repeater_info) def can_execute(self, message: MeshMessage, skip_channel_check: bool = False) -> bool: @@ -475,18 +498,21 @@ class PathCommand(BaseCommand): if len(parts) < 2: # No arguments provided - try to extract path from current message - response = await self._extract_path_from_recent_messages() + response = await self._extract_path_from_recent_messages(message) else: # Extract path data from the command path_input = " ".join(parts[1:]) - response = await self._decode_path(path_input) + response = await self._decode_path(path_input, message=message) # Send the response (may be split into multiple messages if long) await self._send_path_response(message, response) return True async def _decode_path( - self, path_input: str, routing_info: Optional[dict[str, Any]] = None + self, + path_input: str, + routing_info: Optional[dict[str, Any]] = None, + message: Optional[MeshMessage] = None, ) -> str: """Decode hex path data to repeater names. Comma-separated tokens infer hop size (2, 4, or 6 hex chars per node). @@ -517,7 +543,7 @@ class PathCommand(BaseCommand): if not node_ids: return self.translate('commands.path.no_valid_hex') - return await self._decode_node_ids(node_ids, routing_info) + return await self._decode_node_ids(node_ids, routing_info, message=message) except Exception as e: self.logger.error(f"Error decoding path: {e}") @@ -1080,7 +1106,9 @@ class PathCommand(BaseCommand): out = (prefix + current_message.rstrip()) if message_count == 0 else current_message.rstrip() await self.send_response(message, out, skip_user_rate_limit=True) - async def _extract_path_from_recent_messages(self) -> str: + async def _extract_path_from_recent_messages( + self, message: Optional[MeshMessage] = None + ) -> str: """Extract path from the current message's path information (same as test command). Prefers already-extracted routing_info.path_nodes when present (multi-byte path support). """ @@ -1099,7 +1127,7 @@ class PathCommand(BaseCommand): path_nodes = routing_info.get('path_nodes', []) if path_nodes: node_ids = [n.upper() for n in path_nodes] - return await self._decode_node_ids(node_ids, routing_info) + return await self._decode_node_ids(node_ids, routing_info, message=message) # Fallback: parse message.path string (e.g. no routing_info or legacy path) if not msg.path: @@ -1112,7 +1140,7 @@ class PathCommand(BaseCommand): path_part = path_string.split(" via ROUTE_TYPE_")[0] if " via ROUTE_TYPE_" in path_string else path_string if ',' in path_part: - return await self._decode_path(path_part, routing_info) + return await self._decode_path(path_part, routing_info, message=message) hex_pattern = rf'[0-9a-fA-F]{{{getattr(self.bot, "prefix_hex_chars", 2)}}}' if re.search(hex_pattern, path_part): return await self._decode_path(path_part, routing_info) diff --git a/modules/message_handler.py b/modules/message_handler.py index 1e00937..4f74c40 100644 --- a/modules/message_handler.py +++ b/modules/message_handler.py @@ -1548,30 +1548,55 @@ class MessageHandler: self.logger.debug(f"Found exact packet prefix match: {rf_packet_prefix}") return accepted - # Strategy 2: Try pubkey prefix match (for message correlation) + # Strategy 2: Try pubkey prefix match (for message correlation). + # A pubkey prefix identifies a sender, not one transmission. When the cache + # holds several packets from that sender the match is ambiguous, so take the + # newest and mark it non-authoritative rather than attributing its route. if correlation_key: - for data in recent_data: - rf_pubkey_prefix = data.get("pubkey_prefix", "") or "" - if rf_pubkey_prefix == correlation_key: - accepted = _accept(data, RF_MATCH_PUBKEY) - if accepted: - self.logger.debug(f"Found exact pubkey prefix match: {rf_pubkey_prefix}") - return accepted + pubkey_matches = [ + data for data in recent_data + if (data.get("pubkey_prefix", "") or "") == correlation_key + ] + if pubkey_matches: + newest = max(pubkey_matches, key=lambda x: x["timestamp"]) + unique = len(pubkey_matches) == 1 + accepted = _accept(newest, RF_MATCH_PUBKEY if unique else RF_MATCH_FALLBACK) + if accepted: + if unique: + self.logger.debug(f"Found exact pubkey prefix match: {correlation_key}") + else: + self.logger.debug( + "%d cached packets share pubkey prefix %s; using the newest " + "for signal only, not for routing", + len(pubkey_matches), correlation_key, + ) + return accepted - # Strategy 3: Try partial packet prefix matches + # Strategy 3: Try partial packet prefix matches. Same ambiguity caveat as + # above: a shared 16-character prefix is not proof of the same transmission. if correlation_key: + partial_matches = [] for data in recent_data: rf_packet_prefix = data.get("packet_prefix", "") or "" - # Check for partial match (at least 16 characters) min_length = min(len(rf_packet_prefix), len(correlation_key), 16) if rf_packet_prefix[:min_length] == correlation_key[:min_length] and min_length >= 16: - accepted = _accept(data, RF_MATCH_PARTIAL) - if accepted: + partial_matches.append(data) + if partial_matches: + newest = max(partial_matches, key=lambda x: x["timestamp"]) + unique = len(partial_matches) == 1 + accepted = _accept(newest, RF_MATCH_PARTIAL if unique else RF_MATCH_FALLBACK) + if accepted: + if unique: self.logger.debug( - f"Found partial packet prefix match: {rf_packet_prefix[:16]}... " - f"matches {correlation_key[:16]}..." + f"Found partial packet prefix match for {correlation_key[:16]}..." ) - return accepted + else: + self.logger.debug( + "%d cached packets share the partial prefix %s...; using the " + "newest for signal only, not for routing", + len(partial_matches), correlation_key[:16], + ) + return accepted # Strategy 4: Use most recent data (fallback for timing issues) if recent_data: @@ -2329,21 +2354,35 @@ class MessageHandler: scope_packet_info["packet_hash"] = scope_rf_data["packet_hash"] # Scope matching: use scope-eligible RF only (never a stale ADVERT fallback). + # The scope also has to come from *this* message's packet. The HMAC proves + # the cached packet belongs to an allowed scope, not that this message does, + # so an uncorrelated fallback would let a recent allowed-scope packet admit + # an unrelated message past the flood_scopes allowlist. reply_scope: str | None = None cmd_mgr = getattr(self.bot, "command_manager", None) scope_keys = getattr(cmd_mgr, "flood_scope_keys", {}) + scope_rf_is_correlated = rf_data_is_correlated(scope_rf_data) if scope_rf_data and scope_keys: - reply_scope = self._resolve_reply_scope_from_rf_data( - scope_rf_data, scope_packet_info, scope_keys - ) + if scope_rf_is_correlated: + reply_scope = self._resolve_reply_scope_from_rf_data( + scope_rf_data, scope_packet_info, scope_keys + ) + else: + self.logger.info( + "Scope for this channel message is unknown: the only scope-eligible " + "RF data is an uncorrelated fallback from another packet, so it " + "cannot authorise a reply under flood_scopes" + ) # Allowlist enforcement: when flood_scopes is configured, only reply to # messages whose scope matched an entry. Unscoped FLOOD is allowed only # when '*' (or equivalent) is explicitly listed. if scope_keys and reply_scope is None: allow_global = getattr(cmd_mgr, "flood_scope_allow_global", False) - if scope_rf_data and self._is_rf_data_scope_eligible( - scope_rf_data, scope_packet_info + if ( + scope_rf_data + and scope_rf_is_correlated + and self._is_rf_data_scope_eligible(scope_rf_data, scope_packet_info) ): self.logger.info("Ignoring TC_FLOOD: scope not in flood_scopes allowlist") return diff --git a/modules/scheduler.py b/modules/scheduler.py index 20642d1..cd91b6b 100644 --- a/modules/scheduler.py +++ b/modules/scheduler.py @@ -619,10 +619,34 @@ class MessageScheduler: username = "" budget = 160 - len(username.encode("utf-8")) - 2 - if scope and scope.strip(): + if (scope or "").strip(): budget -= CHANNEL_REGIONAL_FLOOD_SCOPE_BODY_OVERHEAD return max(budget, 32) + def _effective_send_scope(self, channel: str, scope: str | None) -> str | None: + """The scope the send will actually use, not just the one on the schedule. + + send_channel_message resolves an unset scope from ``flood_scope.`` and + then ``outgoing_flood_scope_override``. Budgeting on the raw schedule scope + alone would size chunks for a global send and overshoot by the regional header + once the sender adds it. + """ + if (scope or "").strip(): + return scope + try: + resolved = self.bot.command_manager.resolve_channel_send_scope( + scope=None, channel=channel + ) + if (resolved or "").strip(): + return resolved + override = self.bot.config.get( + "Channels", "outgoing_flood_scope_override", fallback="" + ) + return override if (override or "").strip() else None + except Exception: # noqa: BLE001 - budgeting must never break a send + # Unknown means assume regional, which only ever makes chunks smaller. + return "#unknown" + @staticmethod def _split_to_budget(text: str, budget: int) -> list[str]: """Split *text* into chunks of at most *budget* UTF-8 bytes, on line breaks @@ -704,7 +728,8 @@ class MessageScheduler: # A {cmd:...} placeholder can expand to more than one message's worth of text, # and send_channel_message does not split. Chunk to the RF body budget so a # long rendered reply airs as several messages instead of failing at the device. - chunks = self._split_to_budget(message, self._channel_body_budget(scope)) + effective_scope = self._effective_send_scope(channel, scope) + chunks = self._split_to_budget(message, self._channel_body_budget(effective_scope)) if len(chunks) > 1: self.logger.info( "Scheduled message for %s split into %d chunks to fit the RF budget", diff --git a/tests/test_message_handler.py b/tests/test_message_handler.py index 5963af9..196c9ab 100644 --- a/tests/test_message_handler.py +++ b/tests/test_message_handler.py @@ -2219,3 +2219,65 @@ class TestRfCorrelationProvenance: handler.recent_rf_data = [entry] handler.find_recent_rf_data() assert RF_MATCH_KEY not in entry + + +class TestAmbiguousPrefixIsNotAuthoritative: + """A pubkey or partial prefix identifies a sender, not one transmission. When + several cached packets share it the match cannot carry a route (#80 follow-up).""" + + @staticmethod + def _entry(ts_offset, **over): + entry = { + "timestamp": time.time() - ts_offset, + "snr": 5, + "rssi": -80, + "packet_prefix": "", + "pubkey_prefix": "", + } + entry.update(over) + return entry + + def test_single_pubkey_match_is_authoritative(self, handler): + handler.rf_data_timeout = 30 + handler.recent_rf_data = [self._entry(1, pubkey_prefix="abcd")] + result = handler.find_recent_rf_data("abcd") + assert result[RF_MATCH_KEY] == RF_MATCH_PUBKEY + assert rf_data_is_correlated(result) is True + + def test_several_packets_from_one_sender_are_not_authoritative(self, handler): + handler.rf_data_timeout = 30 + handler.recent_rf_data = [ + self._entry(9, pubkey_prefix="abcd", snr=1), + self._entry(1, pubkey_prefix="abcd", snr=2), + ] + result = handler.find_recent_rf_data("abcd") + assert rf_data_is_correlated(result) is False + # Still the newest, so signal figures remain the best available guess. + assert result["snr"] == 2 + + def test_single_partial_match_is_authoritative(self, handler): + handler.rf_data_timeout = 30 + prefix = "aabbccddeeff0011aabbccddeeff0011" + handler.recent_rf_data = [self._entry(1, packet_prefix=prefix)] + result = handler.find_recent_rf_data("aabbccddeeff0011" + "f" * 16) + assert rf_data_is_correlated(result) is True + + def test_several_partial_matches_are_not_authoritative(self, handler): + handler.rf_data_timeout = 30 + handler.recent_rf_data = [ + self._entry(9, packet_prefix="aabbccddeeff0011" + "1" * 16), + self._entry(1, packet_prefix="aabbccddeeff0011" + "2" * 16), + ] + result = handler.find_recent_rf_data("aabbccddeeff0011" + "f" * 16) + assert rf_data_is_correlated(result) is False + + def test_exact_packet_prefix_stays_authoritative_with_others_present(self, handler): + handler.rf_data_timeout = 30 + exact = "deadbeefdeadbeef1234567890abcdef" + handler.recent_rf_data = [ + self._entry(9, pubkey_prefix="abcd"), + self._entry(1, packet_prefix=exact, pubkey_prefix="abcd"), + ] + result = handler.find_recent_rf_data(exact) + assert result[RF_MATCH_KEY] == RF_MATCH_EXACT + assert rf_data_is_correlated(result) is True