fix(mesh): coerce coordinate traffic to the position channel on event builds (#11545)

Under USERPREFS_BLOCK_POSITION_ON_EVENT_CHANNEL every coordinate packet a
client aimed at the event channel was rejected with the "Location sharing is
disabled on this channel" notification - including the phone's own location
feed. Both apps hand a GPS-less node its fix as a POSITION_APP packet
addressed to the node itself on channel 0; that packet never leaves the
device (Router::sendLocal delivers it locally) but resolved to the event
channel and was dropped before PositionModule saw it. Result: the toast on
every location tick, and nodes without a GPS never learned a position to
share on their private channel.

Position traffic now converges on the position channel - findPositionChannel(),
the first channel with non-zero on-wire precision, which is never the event
channel:

- From-us-to-us coordinate packets are exempt from the event block.
- Local coordinate sends aimed at the event channel (phone share-location,
  request-position, waypoints, any module/UI originator) are moved onto the
  position channel in Router::sendLocal and PhoneAPI instead of rejected. The
  client notification is only sent when no channel carries positions at all.
- A position request DM'd to us on the event channel is answered on the
  position channel at that channel's precision (request_id preserved, same
  reply throttle); the requester's coordinates are still not stored,
  forwarded, relayed or published. want_response from the bitfield is merged
  before the event-channel decode short-circuit so such requests are seen.
- PositionModule::sendOurPosition, positionUnchangedSinceLastSend and
  MeshService::trySendPosition use the shared helper instead of three copies
  of the same walk.

Non-event builds are unaffected: the coercion compiles out and the helper
matches the previous walk.

Tests: coverage-event-policy (test_event_channel_phone_api,
test_event_channel_router, test_position_precision, test_mqtt,
test_nexthop_routing) and the same suites with the policy off.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

test/test_event_channel_phone_api/test_main.cpp

@@ -1,4 +1,5 @@
 #include "Channels.h"
+#include "MeshModule.h"
 #include "MeshService.h"
 #include "NodeDB.h"
 #include "RadioInterface.h"
@@ -15,10 +16,28 @@ namespace
 {
 constexpr PacketId BLOCKED_PACKET_ID = 0x10203040;
 constexpr PacketId FOLLOWUP_PACKET_ID = 0x50607080;
+constexpr PacketId WAYPOINT_PACKET_ID = 0x0a0b0c0d;
 constexpr ChannelIndex EVENT_CHANNEL = 0;
 constexpr ChannelIndex PRIVATE_CHANNEL = 1;
+constexpr NodeNum LOCAL_NODE = 0x87654321;
 constexpr NodeNum REMOTE_NODE = 0x12345678;
 
+#if USERPREFS_BLOCK_POSITION_ON_EVENT_CHANNEL && defined(USERPREFS_CHANNEL_0_PSK)
+// Where a coordinate packet the phone aimed at the event channel actually goes once a channel carries positions.
+constexpr ChannelIndex COERCED_CHANNEL = PRIVATE_CHANNEL;
+#else
+constexpr ChannelIndex COERCED_CHANNEL = EVENT_CHANNEL;
+#endif
+
+// Router::sendLocal() loops a to-self packet through MeshModule::callModules(), which walks the module
+// list; construct one so the list exists in this otherwise module-free binary.
+class NoopModule : public MeshModule
+{
+  public:
+    NoopModule() : MeshModule("event-phone-api-noop") {}
+    bool wantPacket(const meshtastic_MeshPacket *) override { return false; }
+};
+
 class MockRadioInterface : public RadioInterface
 {
   public:
@@ -106,6 +125,7 @@ MockMeshService *mockService;
 MockRouter *mockRouter;
 NodeDB *mockNodeDB;
 TestStreamAPI *streamAPI;
+NoopModule *noopModule;
 
 void configureChannels()
 {
@@ -135,20 +155,35 @@ void configureChannels()
     channels.onConfigChanged();
 }
 
-meshtastic_ToRadio makePositionToRadio(PacketId id, ChannelIndex channel)
+// configureChannels() leaves both channels without module_settings, i.e. position sharing off everywhere
+// (getPositionPrecisionForChannel fails closed). Opt the private channel in so it becomes the position channel.
+void enablePositionOnPrivateChannel()
+{
+    auto &privateChannel = channelFile.channels[PRIVATE_CHANNEL];
+    privateChannel.settings.has_module_settings = true;
+    privateChannel.settings.module_settings.position_precision = 32;
+    channels.onConfigChanged();
+}
+
+meshtastic_ToRadio makeCoordinateToRadio(PacketId id, ChannelIndex channel, meshtastic_PortNum portnum, NodeNum to)
 {
     meshtastic_ToRadio message = meshtastic_ToRadio_init_default;
     const meshtastic_MeshPacket defaultPacket = meshtastic_MeshPacket_init_default;
     message.which_payload_variant = meshtastic_ToRadio_packet_tag;
     message.packet = defaultPacket;
-    message.packet.to = REMOTE_NODE;
+    message.packet.to = to;
     message.packet.id = id;
     message.packet.channel = channel;
     message.packet.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
-    message.packet.decoded.portnum = meshtastic_PortNum_POSITION_APP;
+    message.packet.decoded.portnum = portnum;
     return message;
 }
 
+meshtastic_ToRadio makePositionToRadio(PacketId id, ChannelIndex channel)
+{
+    return makeCoordinateToRadio(id, channel, meshtastic_PortNum_POSITION_APP, REMOTE_NODE);
+}
+
 bool sendToRadio(const meshtastic_ToRadio &message)
 {
     uint8_t encoded[meshtastic_ToRadio_size] = {};
@@ -160,13 +195,14 @@ bool sendToRadio(const meshtastic_ToRadio &message)
     return streamAPI->handleToRadio(encoded, encodedSize);
 }
 
-void assertSentPacket(size_t index, PacketId id, ChannelIndex channel)
+void assertSentPacket(size_t index, PacketId id, ChannelIndex channel,
+                      meshtastic_PortNum portnum = meshtastic_PortNum_POSITION_APP)
 {
     TEST_ASSERT_GREATER_THAN(index, mockRouter->sentPackets.size());
     const auto &packet = mockRouter->sentPackets[index];
     TEST_ASSERT_EQUAL_UINT32(id, packet.id);
     TEST_ASSERT_EQUAL_UINT8(channel, packet.channel);
-    TEST_ASSERT_EQUAL(meshtastic_PortNum_POSITION_APP, packet.decoded.portnum);
+    TEST_ASSERT_EQUAL(portnum, packet.decoded.portnum);
 }
 } // namespace
 
@@ -177,16 +213,19 @@ void setUp(void)
 
     service = mockService = new MockMeshService();
     nodeDB = mockNodeDB = new NodeDB();
-    myNodeInfo.my_node_num = 0x87654321;
+    myNodeInfo.my_node_num = LOCAL_NODE;
     configureChannels();
     cryptLock = nullptr; // Router's ctor asserts this is unset before allocating its own.
     router = mockRouter = new MockRouter();
     streamAPI = new TestStreamAPI();
+    noopModule = new NoopModule();
     testDelay(1);
 }
 
 void tearDown(void)
 {
+    delete noopModule;
+    noopModule = nullptr;
     delete streamAPI;
     streamAPI = nullptr;
     delete mockRouter;
@@ -250,12 +289,62 @@ static void test_event_position_ingress_does_not_poison_retry_state()
 #endif
 }
 
+// The apps feed the node its phone GPS fix as a POSITION packet addressed to the node itself on channel 0.
+// That packet never leaves the device, so it must pass regardless of the event policy and without a
+// notification, on any channel configuration (here: no channel carries positions at all).
+static void test_phone_position_to_self_is_never_blocked()
+{
+    const auto toSelf = makeCoordinateToRadio(BLOCKED_PACKET_ID, EVENT_CHANNEL, meshtastic_PortNum_POSITION_APP, LOCAL_NODE);
+
+    TEST_ASSERT_TRUE(sendToRadio(toSelf));
+    TEST_ASSERT_EQUAL(0, mockRouter->sentPackets.size()); // delivered locally, never on the air
+    mockService->assertQueueStatus(BLOCKED_PACKET_ID);
+    TEST_ASSERT_EQUAL(0, mockService->notifications.size());
+}
+
+// A coordinate the phone aims at the event channel is moved onto the position channel (the first channel
+// with position sharing enabled) instead of being rejected, and the phone is not told anything went wrong.
+// Without the event policy the packet stays on the channel the phone chose.
+static void test_phone_coordinates_on_event_channel_move_to_position_channel()
+{
+    enablePositionOnPrivateChannel();
+    const auto positionRequest = makePositionToRadio(BLOCKED_PACKET_ID, EVENT_CHANNEL); // DM (e.g. "request position")
+    const auto waypointBroadcast =
+        makeCoordinateToRadio(WAYPOINT_PACKET_ID, EVENT_CHANNEL, meshtastic_PortNum_WAYPOINT_APP, NODENUM_BROADCAST);
+
+    TEST_ASSERT_TRUE(sendToRadio(positionRequest));
+    TEST_ASSERT_EQUAL(1, mockRouter->sentPackets.size());
+    assertSentPacket(0, BLOCKED_PACKET_ID, COERCED_CHANNEL);
+    mockService->assertQueueStatus(BLOCKED_PACKET_ID);
+    TEST_ASSERT_EQUAL(0, mockService->notifications.size());
+
+    TEST_ASSERT_TRUE(sendToRadio(waypointBroadcast));
+    TEST_ASSERT_EQUAL(2, mockRouter->sentPackets.size());
+    assertSentPacket(1, WAYPOINT_PACKET_ID, COERCED_CHANNEL, meshtastic_PortNum_WAYPOINT_APP);
+    mockService->assertQueueStatus(WAYPOINT_PACKET_ID);
+    TEST_ASSERT_EQUAL(0, mockService->notifications.size());
+}
+
+// A coordinate already on the position channel is left alone.
+static void test_phone_coordinates_on_position_channel_are_untouched()
+{
+    enablePositionOnPrivateChannel();
+
+    TEST_ASSERT_TRUE(sendToRadio(makePositionToRadio(FOLLOWUP_PACKET_ID, PRIVATE_CHANNEL)));
+    TEST_ASSERT_EQUAL(1, mockRouter->sentPackets.size());
+    assertSentPacket(0, FOLLOWUP_PACKET_ID, PRIVATE_CHANNEL);
+    TEST_ASSERT_EQUAL(0, mockService->notifications.size());
+}
+
 extern "C" {
 void setup()
 {
     initializeTestEnvironment();
     UNITY_BEGIN();
     RUN_TEST(test_event_position_ingress_does_not_poison_retry_state);
+    RUN_TEST(test_phone_position_to_self_is_never_blocked);
+    RUN_TEST(test_phone_coordinates_on_event_channel_move_to_position_channel);
+    RUN_TEST(test_phone_coordinates_on_position_channel_are_untouched);
     exit(UNITY_END());
 }
 

test/test_event_channel_router/test_main.cpp

@@ -9,7 +9,11 @@
 #include "mesh/MeshRadio.h"
 #include "mesh/MeshService.h"
 #include "mesh/NodeDB.h"
+#include "mesh/PositionPrecision.h"
 #include "mesh/Router.h"
+#include "modules/PositionModule.h"
+#include "modules/RoutingModule.h"
+#include "support/MockMeshService.h"
 #include <array>
 #include <cstdio>
 #include <cstring>
@@ -277,6 +281,151 @@ static void test_opaque_tx_is_not_misclassified_as_coordinates()
     TEST_ASSERT_EQUAL_UINT32(1, captureRadio->packets.size());
 }
 
+#if USERPREFS_BLOCK_POSITION_ON_EVENT_CHANNEL
+static void enablePositionOnPrivateChannel()
+{
+    meshtastic_Channel &privateChannel = channelFile.channels[kPrivateChannel];
+    privateChannel.settings.has_module_settings = true;
+    privateChannel.settings.module_settings.position_precision = 32;
+    channels.onConfigChanged();
+    uint8_t positionChannel = 0xff;
+    TEST_ASSERT_TRUE(findPositionChannel(positionChannel));
+    TEST_ASSERT_EQUAL_UINT8(kPrivateChannel, positionChannel);
+}
+
+// The reply path needs the module, a service to send through, a routing module for the response hop
+// limit, and a fix of our own. Scoped to the one test so the rest of the suite stays module-free.
+struct ReplyHarness {
+    MeshService *savedService = service;
+    RoutingModule *savedRouting = routingModule;
+    PositionModule *savedPosition = positionModule;
+    MockMeshService localService;
+    RoutingModule localRouting;
+    PositionModule localPosition;
+
+    ReplyHarness()
+    {
+        service = &localService;
+        routingModule = &localRouting;
+        positionModule = &localPosition;
+        testNodeDB->addNode(kLocalNode, kEventChannel); // refreshLocalMeshNode() asserts our own entry exists
+        meshtastic_Position fix = meshtastic_Position_init_zero;
+        fix.has_latitude_i = true;
+        fix.latitude_i = 407825770;
+        fix.has_longitude_i = true;
+        fix.longitude_i = -1192084390;
+        testNodeDB->setLocalPosition(fix);
+    }
+
+    ~ReplyHarness()
+    {
+        // Drain what sendToMesh() queued for the (absent) phone so the pools are clean at exit.
+        while (auto *status = localService.getQueueStatusForPhone())
+            localService.releaseQueueStatusToPool(status);
+        while (auto *packet = localService.getForPhone())
+            localService.releaseToPool(packet);
+        positionModule = savedPosition;
+        routingModule = savedRouting;
+        service = savedService;
+    }
+};
+
+// A position request DM'd to us on the event channel is not processed (no module sees it, so nothing is
+// stored or forwarded), but it is answered: our position goes out as a reply, on the position channel.
+static void test_rx_event_channel_position_request_to_us_is_answered_on_position_channel()
+{
+    enablePositionOnPrivateChannel();
+    ReplyHarness harness;
+
+    meshtastic_MeshPacket request = makeDecodedPacket(meshtastic_PortNum_POSITION_APP, kRemoteNode, kLocalNode, kEventChannel);
+    request.decoded.want_response = true;
+    receivePacket(request);
+
+    TEST_ASSERT_EQUAL_UINT32(0, captureModule->packets.size());
+    TEST_ASSERT_EQUAL_UINT32(1, captureRadio->packets.size());
+
+    meshtastic_MeshPacket reply = captureRadio->packets.front();
+    TEST_ASSERT_EQUAL_UINT32(kRemoteNode, reply.to);
+    TEST_ASSERT_EQUAL_UINT32(kLocalNode, reply.from);
+    TEST_ASSERT_EQUAL(meshtastic_MeshPacket_encrypted_tag, reply.which_payload_variant); // went out under a channel key
+    TEST_ASSERT_EQUAL(DecodeState::DECODE_SUCCESS, perhapsDecode(&reply));
+    TEST_ASSERT_EQUAL_UINT8(kPrivateChannel, reply.channel); // ...the position channel's, not the event channel's
+    TEST_ASSERT_EQUAL(meshtastic_PortNum_POSITION_APP, reply.decoded.portnum);
+    TEST_ASSERT_EQUAL_UINT32(request.id, reply.decoded.request_id);
+}
+
+// Without a position channel there is nothing to answer on: the request is simply dropped.
+static void test_rx_event_channel_position_request_without_position_channel_is_dropped()
+{
+    ReplyHarness harness;
+
+    meshtastic_MeshPacket request = makeDecodedPacket(meshtastic_PortNum_POSITION_APP, kRemoteNode, kLocalNode, kEventChannel);
+    request.decoded.want_response = true;
+    receivePacket(request);
+
+    TEST_ASSERT_EQUAL_UINT32(0, captureModule->packets.size());
+    TEST_ASSERT_EQUAL_UINT32(0, captureRadio->packets.size());
+}
+
+// A broadcast position on the event channel is dropped outright, want_response or not: only unicast
+// requests to us are answered.
+static void test_rx_event_channel_position_broadcast_with_want_response_is_not_answered()
+{
+    enablePositionOnPrivateChannel();
+    ReplyHarness harness;
+
+    meshtastic_MeshPacket broadcast =
+        makeDecodedPacket(meshtastic_PortNum_POSITION_APP, kRemoteNode, NODENUM_BROADCAST, kEventChannel);
+    broadcast.decoded.want_response = true;
+    receivePacket(broadcast);
+
+    TEST_ASSERT_EQUAL_UINT32(0, captureModule->packets.size());
+    TEST_ASSERT_EQUAL_UINT32(0, captureRadio->packets.size());
+}
+
+// The phone hands a GPS-less node its fix as a POSITION packet from us to us on channel 0. It never goes on
+// the air, so the event policy must let it through to the modules (where PositionModule records it).
+static void test_loopback_position_from_us_to_us_on_event_channel_is_not_blocked()
+{
+    meshtastic_MeshPacket loopback = makeDecodedPacket(meshtastic_PortNum_POSITION_APP, kLocalNode, kLocalNode, kEventChannel);
+    TEST_ASSERT_FALSE(isBlockedEventCoordinatePacket(&loopback));
+
+    meshtastic_MeshPacket *packet = packetPool.allocCopy(loopback);
+    TEST_ASSERT_NOT_NULL(packet);
+    TEST_ASSERT_EQUAL_INT(ERRNO_SHOULD_RELEASE, testRouter->sendLocal(packet, RX_SRC_USER));
+    packetPool.release(packet);
+
+    TEST_ASSERT_EQUAL_UINT32(1, captureModule->packets.size());
+    TEST_ASSERT_EQUAL_UINT32(0, captureRadio->packets.size());
+}
+
+// A local originator (module, UI) that aims a coordinate at the event channel is moved onto the position
+// channel by sendLocal(); with no position channel the send is still refused.
+static void test_tx_local_coordinate_on_event_channel_is_moved_to_position_channel()
+{
+    meshtastic_MeshPacket *packet = testRouter->allocForSending();
+    TEST_ASSERT_NOT_NULL(packet);
+    packet->to = NODENUM_BROADCAST;
+    packet->channel = kEventChannel;
+    packet->decoded = makeDecodedPacket(meshtastic_PortNum_POSITION_APP, kLocalNode, NODENUM_BROADCAST, kEventChannel).decoded;
+    TEST_ASSERT_EQUAL_INT(meshtastic_Routing_Error_NOT_AUTHORIZED, testRouter->sendLocal(packet, RX_SRC_LOCAL));
+    TEST_ASSERT_EQUAL_UINT32(0, captureRadio->packets.size());
+
+    enablePositionOnPrivateChannel();
+    packet = testRouter->allocForSending();
+    TEST_ASSERT_NOT_NULL(packet);
+    packet->to = NODENUM_BROADCAST;
+    packet->channel = kEventChannel;
+    packet->decoded = makeDecodedPacket(meshtastic_PortNum_POSITION_APP, kLocalNode, NODENUM_BROADCAST, kEventChannel).decoded;
+    TEST_ASSERT_EQUAL_INT(ERRNO_OK, testRouter->sendLocal(packet, RX_SRC_LOCAL));
+    TEST_ASSERT_EQUAL_UINT32(1, captureRadio->packets.size());
+
+    meshtastic_MeshPacket sent = captureRadio->packets.front();
+    TEST_ASSERT_EQUAL(DecodeState::DECODE_SUCCESS, perhapsDecode(&sent));
+    TEST_ASSERT_EQUAL_UINT8(kPrivateChannel, sent.channel);
+}
+#endif
+
 static void test_capture_endpoints_release_packet_pool_ownership()
 {
     constexpr size_t iterations = 64;
@@ -382,6 +531,13 @@ EVENT_ROUTER_TEST_ENTRY void setup()
     RUN_TEST(test_tx_event_coordinate_that_uses_pki_reaches_radio);
 #endif
     RUN_TEST(test_opaque_tx_is_not_misclassified_as_coordinates);
+#if USERPREFS_BLOCK_POSITION_ON_EVENT_CHANNEL
+    RUN_TEST(test_rx_event_channel_position_request_to_us_is_answered_on_position_channel);
+    RUN_TEST(test_rx_event_channel_position_request_without_position_channel_is_dropped);
+    RUN_TEST(test_rx_event_channel_position_broadcast_with_want_response_is_not_answered);
+    RUN_TEST(test_loopback_position_from_us_to_us_on_event_channel_is_not_blocked);
+    RUN_TEST(test_tx_local_coordinate_on_event_channel_is_moved_to_position_channel);
+#endif
     RUN_TEST(test_capture_endpoints_release_packet_pool_ownership);
 
     exit(UNITY_END());

test/test_position_precision/test_main.cpp

@@ -304,6 +304,9 @@ static meshtastic_MeshPacket makeDecodedPacket(meshtastic_PortNum portnum, uint8
     packet.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
     packet.decoded.portnum = portnum;
     packet.channel = channelIndex;
+    // A real destination: this suite never sets a node number, so a default to=0 would read as
+    // "to us" (getNodeNum()==0) and take the from-us-to-us loopback exemption.
+    packet.to = NODENUM_BROADCAST;
     return packet;
 }
 
@@ -373,7 +376,12 @@ static void test_eventCoordinatePolicy_usesResolvedUnicastChannel()
     configureEventChannels(false, false);
     meshtastic_NodeInfoLite *node =
         nodeDB->getNumMeshNodes() > 1 ? nodeDB->getMeshNodeByIndex(1) : nodeDB->getOrCreateMeshNode(0x12345678);
+    // A persisted DB (unsandboxed host run) can hand back our own entry here; a from-us-to-us packet is
+    // loopback-exempt, which is not the policy under test. Insist on a remote destination.
+    if (node && node->num == nodeDB->getNodeNum())
+        node = nodeDB->getOrCreateMeshNode(0x12345678);
     TEST_ASSERT_NOT_NULL(node);
+    TEST_ASSERT_NOT_EQUAL(nodeDB->getNodeNum(), node->num);
     const NodeNum destination = node->num;
     const uint8_t savedChannel = node->channel;
 
@@ -396,6 +404,38 @@ static void test_eventCoordinatePolicy_usesResolvedUnicastChannel()
 #endif
 }
 
+static void test_findPositionChannel_skipsEventAndDisabledChannels()
+{
+    // Both channels store precision 16. Under the block gate the event channel never carries
+    // positions, so the private one (index 1) is the position channel; otherwise index 0 wins.
+    configureEventChannels(false, false);
+    uint8_t positionChannel = 0xff;
+    TEST_ASSERT_TRUE(findPositionChannel(positionChannel));
+#if USERPREFS_BLOCK_POSITION_ON_EVENT_CHANNEL && defined(USERPREFS_CHANNEL_0_PSK)
+    TEST_ASSERT_EQUAL_UINT8(1, positionChannel);
+#else
+    TEST_ASSERT_EQUAL_UINT8(0, positionChannel);
+#endif
+
+    // Reordering follows the effective key, not the index.
+    configureEventChannels(true, false);
+    TEST_ASSERT_TRUE(findPositionChannel(positionChannel));
+    TEST_ASSERT_EQUAL_UINT8(0, positionChannel);
+
+    // Precision 0 everywhere: nothing to pick.
+    configureEventChannels(false, false);
+    channelFile.channels[0].settings.module_settings.position_precision = 0;
+    channelFile.channels[1].settings.module_settings.position_precision = 0;
+    channels.onConfigChanged();
+    TEST_ASSERT_FALSE(findPositionChannel(positionChannel));
+
+    // A disabled channel does not count even with a stored precision.
+    channelFile.channels[1].settings.module_settings.position_precision = 32;
+    channelFile.channels[1].role = meshtastic_Channel_Role_DISABLED;
+    channels.onConfigChanged();
+    TEST_ASSERT_FALSE(findPositionChannel(positionChannel));
+}
+
 static void test_getPositionPrecisionForChannel_nonEventFullKeyIsHonored()
 {
     // A private channel with a full 32-byte key that is not the configured
@@ -439,6 +479,7 @@ void setup()
     RUN_TEST(test_eventCoordinatePolicy_coversPortsAndExcludesPki);
     RUN_TEST(test_eventCoordinatePolicy_doesNotClassifyOpaquePacketsByHash);
     RUN_TEST(test_eventCoordinatePolicy_usesResolvedUnicastChannel);
+    RUN_TEST(test_findPositionChannel_skipsEventAndDisabledChannels);
     RUN_TEST(test_getPositionPrecisionForChannel_nonEventFullKeyIsHonored);
     exit(UNITY_END());
 }