test(native): add 14 suites for routing, persistence, parsing and identity gaps (#11515)

* test(native): add 14 suites for routing, persistence, parsing and identity gaps Coverage audit of the native test tree; adds the highest-value untested logic as 11 new suites and extends 3 existing ones (200 test functions). New: test_stream_framing, test_nodedb_boot_recovery, test_nodedb_legacy_migration, test_nodedb_v25_roundtrip, test_nodedb_identity_hygiene, test_channel_keys, test_reliable_ack_matrix, test_hop_start_policy, test_routing_response_hops, test_phone_api_config_dump, test_observer. Extended: test_rtc, test_mqtt, test_xmodem. Two source changes the audit produced: - StreamAPI::handleRecStream copied stream->read()'s `cInt < 0` EOF check into the buffer-fed path, where there is no EOF sentinel; with signed char any byte >= 0x80 (START1 is 0x94) aborted the parse. Read the byte as uint8_t directly. Latent on develop (no callers), pinned by test_stream_framing. - Extract the post-decode pre-hop predicate from Router::handleReceived into shouldSkipHandleForPostDecodeHop() (NodeDB.h) so test_hop_start_policy drives the exact expression the router calls. No behavior change. test/state-manifest.tsv declares the suites that construct a NodeDB. Full 68-suite Docker coverage run matches the pre-change baseline. * test(native): address review - harden observer dispatch, trim comments Review follow-ups on the coverage-audit suites: - Observable::notifyObservers() erased list nodes while holding an iterator into them, so an observer that unobserves itself from onNotify corrupted the dispatch. Today the only self-detacher (PhoneAPI::onNotify -> checkConnectionTimeout -> close -> unobserve) survives solely because it returns -1 and aborts the chain before the increment; that unwritten contract is now gone. Removal during a dispatch nulls the entry and the outermost notify sweeps afterwards, which keeps self-detach, next-detach and destruction-during-notify all safe without an allocation. Hoisting the next iterator instead would have inverted the hazard and broken the existing next-detach case. Two regression tests added. - Correct the documented caller of shouldSkipHandleForPostDecodeHop: the call is in Router::dispatchReceived, not handleReceived. - Cast hop fields to unsigned at the %u call site in test_hop_start_policy. - Trim the new suites' file headers to the one-or-two-line rule in AGENTS.md. - Rename eight test functions whose names were exactly `test_` + 35 chars: that is the shape of a Lob API key, so trufflehog flagged them as secrets and failed the Trunk CI check. Full 68-suite Docker coverage run matches the pre-change baseline. * test(native): revert the observer dispatch change, keep the contract test Backs out the notifyObservers() deferred-removal hardening from the previous commit. It was reviewer-driven scope creep: nothing in the coverage audit needed it, no test required it, and it changes dispatch semantics in a header with ~76 observe() call sites on native verification alone. The hazard it addressed is not reachable today. The only observer that unobserves itself from onNotify is PhoneAPI (onNotify -> checkConnectionTimeout -> close -> unobserve), and it returns -1, which aborts the chain before the iterator is advanced past the erased node. test_self_detach_with_abort_during_notify stays: it passes against the unmodified dispatch and pins that the -1 is load-bearing, so a later cleanup that "simplifies" it away goes red. The unsafe variant (self-detach returning 0) is documented in a comment rather than tested, since asserting it would be asserting UB. * fix(serial): recover the frame behind a stray framing marker A byte that failed the START2 check was discarded rather than re-tested as a possible START1, so 0x94 0x94 0xc3 ... lost the real frame: one corrupted byte on a noisy UART silently dropped the frame behind it. Re-test the byte in place instead. Applied to both copies of the receive state machine. readStream() is the one that matters in the field - it is the serial path every phone client uses - while handleRecStream() still has no callers on develop. Strictly widens what the parser accepts; no frame that parsed before parses differently. test_stream_framing covers it on both receive paths, plus a run of stray markers and a START1-then-unrelated-byte resync. This was originally documented as a known gap in the framing suite. Fixing it instead was NomDeTom's call on review: a passing test asserting the bad behavior is what makes it hard to change later, and it is the same defect shape as the signedness fix three functions away. Also: use Throttle::deadlinePassed() in test_reliable_ack_matrix rather than a bare millis() compare, matching the house deadline rule. * test(native): cover the stray-marker resync on the buffer path too The stray-marker fix went into both copies of the receive state machine, but only test_stray_start1_before_frame_still_delivers drove both. The repeated- marker and unrelated-byte cases drove readStream() alone, so a regression in handleRecStream() would have gone unnoticed by two of the three. Verified load-bearing: reverting only the handleRecStream() half of the fix turns test_repeated_stray_start1_before_frame_still_delivers red on the new assertion. test_start1_then_unrelated_byte_resyncs stays green under that mutation by design - its failing byte is 0x00, where both branches reset to 0 - and covers the other half of the ternary. Also drops the stale header on test_stray_start1_before_frame_still_delivers, which still described the gap as pinned-as-is after the fix landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(native): make the hop-start truth table assert the rows it prints test_truth_table_summary was six TEST_MESSAGE lines and no assertion, so it reported as a case that could not fail - the anti-pattern #11517 names in its unfinished assertion-presence lint, and the one exception to NomDeTom's "no RUN_TEST without an assertion" pass over this PR. The printed row and the checked expectation now come from one struct, so the summary cannot narrate a table the predicates no longer implement. It also covers the consequence columns the per-row tests do not assert together: classifyHopStart, shouldDropPacketForPreHop and shouldSkipHandleForPostDecodeHop for the same packet, with the expectations gated on MESHTASTIC_PREHOP_DROP. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-10-22 11:17:49 +10:00 · 2026-08-18 12:41:08 +00:00
parent e1ea653a45
commit 83fd62b756
18 changed files with 6710 additions and 30 deletions
@@ -27,6 +27,7 @@

 #include <algorithm>
 #include <list>
+#include <map>
 #include <optional>
 #include <set>
 #include <sstream>
@@ -86,8 +87,15 @@ class MockMeshService : public MeshService
 class MockNodeDB : public NodeDB
 {
  public:
-    meshtastic_NodeInfoLite *getMeshNode(NodeNum n) override { return &emptyNode; }
+    // Per-NodeNum overlay on top of the shared node, so a test can make one endpoint known
+    // while another stays unknown; everything else keeps the shared-node semantics.
+    meshtastic_NodeInfoLite *getMeshNode(NodeNum n) override
+    {
+        auto it = nodes_.find(n);
+        return it != nodes_.end() ? &it->second : &emptyNode;
+    }
    meshtastic_NodeInfoLite emptyNode = {};
+    std::map<NodeNum, meshtastic_NodeInfoLite> nodes_;
 };

 // Minimal RoutingModule needed to return values from sendAckNak.
@@ -417,8 +425,10 @@ void setUp(void)

    // The shared MockNodeDB node is mutated by the XEdDSA policy tests (signer bit, public
    // key); reset it so state can't leak between tests.
-    if (mockNodeDB)
+    if (mockNodeDB) {
        mockNodeDB->emptyNode = meshtastic_NodeInfoLite();
+        mockNodeDB->nodes_.clear();
+    }

    router = mockRouter = new MockRouter();
    service = mockMeshService = new MockMeshService();
@@ -758,7 +768,9 @@ void test_receiveIgnoresOwnPublishedMessages(void)
    TEST_ASSERT_TRUE(mockRoutingModule->ackNacks_.empty());
 }

-// Considers receiving one of our packets an acknowledgement of it being sent.
+// Considers receiving one of our packets an acknowledgement of it being sent: hearing our own
+// packet back on our own gateway topic synthesizes an implicit ACK, delivered locally through
+// sendLocal() -> handleReceived() -> the phone queue, marked as arriving via MQTT transport.
 void test_receiveAcksOwnSentMessages(void)
 {
    meshtastic_MeshPacket p = decoded;
@@ -766,13 +778,26 @@ void test_receiveAcksOwnSentMessages(void)

    unitTest->publish(&p, nodeDB->getNodeId().c_str());

-    // FIXME: Better assertion for this test
-    // TEST_ASSERT_TRUE(mockRouter->packets_.empty());
-    // TEST_ASSERT_EQUAL(1, mockRoutingModule->ackNacks_.size());
-    // const auto &[err, to, idFrom, chIndex, hopLimit] = mockRoutingModule->ackNacks_.front();
-    // TEST_ASSERT_EQUAL(meshtastic_Routing_Error_NONE, err);
-    // TEST_ASSERT_EQUAL(myNodeInfo.my_node_num, to);
-    // TEST_ASSERT_EQUAL(p.id, idFrom);
+    // The implicit ACK is delivered locally, never enqueued as MQTT downlink ingress.
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+
+    meshtastic_MeshPacket *ack = mockMeshService->getForPhone();
+    TEST_ASSERT_NOT_NULL(ack);
+    TEST_ASSERT_EQUAL(meshtastic_MeshPacket_decoded_tag, ack->which_payload_variant);
+    TEST_ASSERT_EQUAL(meshtastic_PortNum_ROUTING_APP, ack->decoded.portnum);
+    TEST_ASSERT_EQUAL(myNodeInfo.my_node_num, ack->to);
+    TEST_ASSERT_EQUAL(myNodeInfo.my_node_num, ack->from);
+    TEST_ASSERT_EQUAL(p.id, ack->decoded.request_id);
+    TEST_ASSERT_EQUAL(meshtastic_MeshPacket_TransportMechanism_TRANSPORT_MQTT, ack->transport_mechanism);
+
+    meshtastic_Routing routing = meshtastic_Routing_init_default;
+    TEST_ASSERT_TRUE(
+        pb_decode_from_bytes(ack->decoded.payload.bytes, ack->decoded.payload.size, &meshtastic_Routing_msg, &routing));
+    TEST_ASSERT_EQUAL(meshtastic_Routing_error_reason_tag, routing.which_variant);
+    TEST_ASSERT_EQUAL(meshtastic_Routing_Error_NONE, routing.error_reason);
+
+    mockMeshService->releaseToPool(ack);
+    TEST_ASSERT_NULL(mockMeshService->getForPhone()); // exactly one ACK
 }

 // Should ignore our own messages from MQTT that were heard by other nodes.
@@ -967,6 +992,208 @@ void test_receiveIgnoresInvalidHopLimit(void)
    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
 }

+// ===========================================================================
+// Downlink acceptance gates - shouldDropMqttDownlink + onReceiveProto policy
+// ===========================================================================
+
+// hop_start above HOP_MAX is rejected even when hop_limit is valid.
+void test_receiveIgnoresInvalidHopStart(void)
+{
+    meshtastic_MeshPacket p = decoded;
+    p.hop_start = 10;
+    p.hop_limit = 3;
+
+    unitTest->publish(&p);
+
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+}
+
+// The ignore_mqtt kill-switch drops every MQTT downlink.
+void test_receiveDropsWhenIgnoreMqttSet(void)
+{
+    config.lora.ignore_mqtt = true;
+
+    unitTest->publish(&decoded);
+
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+}
+
+// A sender listed in config.lora.ignore_incoming is dropped.
+void test_receiveDropsSenderInIgnoreIncomingList(void)
+{
+    config.lora.ignore_incoming_count = 1;
+    config.lora.ignore_incoming[0] = decoded.from;
+
+    unitTest->publish(&decoded);
+
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+}
+
+// A non-empty ignore list only drops matching senders - presence of the list alone must not drop.
+void test_receiveAcceptsSenderNotInIgnoreIncomingList(void)
+{
+    config.lora.ignore_incoming_count = 2;
+    config.lora.ignore_incoming[0] = 99;
+    config.lora.ignore_incoming[1] = 100;
+
+    unitTest->publish(&decoded);
+
+    TEST_ASSERT_EQUAL(1, mockRouter->packets_.size());
+}
+
+// A sender whose NodeDB entry carries the is_ignored bit is dropped (resurrect-ignored-node guard).
+void test_receiveDropsNodeDbIgnoredSender(void)
+{
+    mockNodeDB->emptyNode.bitfield |= NODEINFO_BITFIELD_IS_IGNORED_MASK;
+
+    unitTest->publish(&decoded);
+
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+}
+
+// A packet claiming the broadcast address as its source is dropped.
+void test_receiveDropsBroadcastSource(void)
+{
+    meshtastic_MeshPacket p = decoded;
+    p.from = NODENUM_BROADCAST;
+
+    unitTest->publish(&p);
+
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+    TEST_ASSERT_TRUE(mockRoutingModule->ackNacks_.empty());
+}
+
+// A broker cannot assert PKI authentication or a transport: every accepted downlink is laundered
+// to pki_encrypted=false + TRANSPORT_MQTT + via_mqtt=true. pki_encrypted grants admin-level trust
+// downstream, so a regression here is remote privilege escalation.
+void test_receiveLaundersPkiAndTransportFields(void)
+{
+    meshtastic_MeshPacket p = decoded;
+    p.pki_encrypted = true;
+    p.transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA;
+
+    unitTest->publish(&p);
+
+    TEST_ASSERT_EQUAL(1, mockRouter->packets_.size());
+    const meshtastic_MeshPacket &r = mockRouter->packets_.front();
+    TEST_ASSERT_FALSE(r.pki_encrypted);
+    TEST_ASSERT_EQUAL(meshtastic_MeshPacket_TransportMechanism_TRANSPORT_MQTT, r.transport_mechanism);
+    TEST_ASSERT_TRUE(r.via_mqtt);
+}
+
+// PKI-topic envelopes are dropped when no channel has downlink enabled, even when addressed to us.
+void test_receiveDropsPkiTopicWhenNoChannelHasDownlink(void)
+{
+    channelFile.channels[0].settings.downlink_enabled = false;
+    meshtastic_MeshPacket e = encrypted;
+    e.to = myNodeInfo.my_node_num;
+
+    unitTest->publish(&e, "!87654321", "PKI");
+
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+}
+
+// Any single downlink-enabled channel (here only a secondary) is enough to admit PKI envelopes.
+void test_receiveAcceptsPkiTopicWithOnlySecondaryDownlink(void)
+{
+    channelFile.channels[0].settings.downlink_enabled = false;
+    channelFile.channels[1] = meshtastic_Channel{
+        .index = 1,
+        .has_settings = true,
+        .settings = {.name = "second", .downlink_enabled = true},
+        .role = meshtastic_Channel_Role_SECONDARY,
+    };
+    channelFile.channels_count = 2;
+    channels.onConfigChanged();
+    meshtastic_MeshPacket e = encrypted;
+    e.to = myNodeInfo.my_node_num;
+
+    unitTest->publish(&e, "!87654321", "PKI");
+
+    TEST_ASSERT_EQUAL(1, mockRouter->packets_.size());
+}
+
+// An encrypted PKI envelope not addressed to us needs both endpoints known with user info.
+void test_receiveDropsPkiNotToUsWithUnknownEndpoints(void)
+{
+    unitTest->publish(&encrypted, "!87654321", "PKI"); // to=2; neither endpoint has user info
+
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+}
+
+void test_receiveAcceptsPkiNotToUsWithKnownEndpoints(void)
+{
+    // MockNodeDB serves the same node for every NodeNum, so this marks both endpoints known.
+    mockNodeDB->emptyNode.bitfield |= NODEINFO_BITFIELD_HAS_USER_MASK;
+
+    unitTest->publish(&encrypted, "!87654321", "PKI");
+
+    TEST_ASSERT_EQUAL(1, mockRouter->packets_.size());
+    const meshtastic_MeshPacket &r = mockRouter->packets_.front();
+    TEST_ASSERT_TRUE(r.via_mqtt);
+    TEST_ASSERT_FALSE(r.pki_encrypted); // laundered even on the PKI topic
+    TEST_ASSERT_EQUAL(meshtastic_MeshPacket_TransportMechanism_TRANSPORT_MQTT, r.transport_mechanism);
+}
+
+// The endpoint gate is an AND: knowing only the sender (from=1) while the receiver (to=2) is
+// unknown must still drop. Distinguishes && from || in the MQTT.cpp acceptance rule.
+void test_receiveDropsPkiNotToUsWithOnlySenderKnown(void)
+{
+    mockNodeDB->nodes_[1].bitfield |= NODEINFO_BITFIELD_HAS_USER_MASK; // only from=1 known; to=2 stays unknown
+
+    unitTest->publish(&encrypted, "!87654321", "PKI");
+
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+}
+
+// An envelope naming a channel we do not have is dropped, even though getByName falls back to
+// the primary channel - the case-sensitive global-id recheck must refuse the substitution.
+void test_receiveDropsUnknownChannelName(void)
+{
+    unitTest->publish(&decoded, "!87654321", "nope");
+
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+}
+
+// getByName matches case-insensitively, but the downlink gate compares case-sensitively; a
+// mixed-case channel_id must not ride the primary channel's downlink permission.
+void test_receiveDropsCaseMismatchedChannelName(void)
+{
+    unitTest->publish(&decoded, "!87654321", "TEST");
+
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+}
+
+// A validly-decoding envelope missing channel_id is rejected before any gate runs.
+void test_receiveRejectsEnvelopeWithoutChannelId(void)
+{
+    const meshtastic_ServiceEnvelope env = {.packet = const_cast<meshtastic_MeshPacket *>(&decoded),
+                                            .channel_id = NULL,
+                                            .gateway_id = const_cast<char *>("!87654321")};
+    uint8_t bytes[256];
+    const size_t numBytes = pb_encode_to_bytes(bytes, sizeof(bytes), &meshtastic_ServiceEnvelope_msg, &env);
+    unitTest->deliverRaw("msh/2/e/test/!87654321", bytes, numBytes);
+
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+}
+
+// Every strict prefix of a valid envelope must be rejected: either the truncated decode fails, or
+// it succeeds with gateway_id (the last-encoded field) missing and the NULL check refuses it.
+void test_receiveRejectsTruncatedEnvelope(void)
+{
+    const meshtastic_ServiceEnvelope env = {.packet = const_cast<meshtastic_MeshPacket *>(&decoded),
+                                            .channel_id = const_cast<char *>("test"),
+                                            .gateway_id = const_cast<char *>("!87654321")};
+    uint8_t bytes[256];
+    const size_t numBytes = pb_encode_to_bytes(bytes, sizeof(bytes), &meshtastic_ServiceEnvelope_msg, &env);
+    TEST_ASSERT_TRUE(numBytes > 0);
+
+    for (size_t n = 1; n < numBytes; n++)
+        unitTest->deliverRaw("msh/2/e/test/!87654321", bytes, n);
+
+    TEST_ASSERT_TRUE(mockRouter->packets_.empty());
+}
+
 // Publishing to a text channel.
 void test_publishTextMessageDirect(void)
 {
@@ -1295,6 +1522,22 @@ void setup()
 #endif
    RUN_TEST(test_receiveIgnoresUnexpectedFields);
    RUN_TEST(test_receiveIgnoresInvalidHopLimit);
+    RUN_TEST(test_receiveIgnoresInvalidHopStart);
+    RUN_TEST(test_receiveDropsWhenIgnoreMqttSet);
+    RUN_TEST(test_receiveDropsSenderInIgnoreIncomingList);
+    RUN_TEST(test_receiveAcceptsSenderNotInIgnoreIncomingList);
+    RUN_TEST(test_receiveDropsNodeDbIgnoredSender);
+    RUN_TEST(test_receiveDropsBroadcastSource);
+    RUN_TEST(test_receiveLaundersPkiAndTransportFields);
+    RUN_TEST(test_receiveDropsPkiTopicWhenNoChannelHasDownlink);
+    RUN_TEST(test_receiveAcceptsPkiTopicWithOnlySecondaryDownlink);
+    RUN_TEST(test_receiveDropsPkiNotToUsWithUnknownEndpoints);
+    RUN_TEST(test_receiveAcceptsPkiNotToUsWithKnownEndpoints);
+    RUN_TEST(test_receiveDropsPkiNotToUsWithOnlySenderKnown);
+    RUN_TEST(test_receiveDropsUnknownChannelName);
+    RUN_TEST(test_receiveDropsCaseMismatchedChannelName);
+    RUN_TEST(test_receiveRejectsEnvelopeWithoutChannelId);
+    RUN_TEST(test_receiveRejectsTruncatedEnvelope);
    RUN_TEST(test_receiveFuzzServiceEnvelope);
    RUN_TEST(test_publishTextMessageDirect);
    RUN_TEST(test_publishTextMessageWithProxy);