From 982f96ca96ba1055f402cec9cbec33644bda6573 Mon Sep 17 00:00:00 2001 From: mikecarper Date: Mon, 5 Oct 2026 01:47:05 -0700 Subject: [PATCH] Avoid Companion request tag collisions after login --- docs/releases/1.17.1.9.md | 4 + examples/companion_radio/MyMesh.cpp | 14 ++++ examples/companion_radio/MyMesh.h | 1 + src/helpers/BaseChatMesh.cpp | 14 ++-- src/helpers/BaseChatMesh.h | 1 + .../companion_delayed_reply_delivery/test.cpp | 73 ++++++++++++++++--- .../contact_cache/test_contact_cache.cpp | 1 + test/test_client_acl_response.py | 2 +- test/test_companion_delayed_reply_delivery.py | 4 +- test/test_contact_cache.py | 1 + test/test_official_app_compatibility.py | 10 +++ 11 files changed, 106 insertions(+), 19 deletions(-) diff --git a/docs/releases/1.17.1.9.md b/docs/releases/1.17.1.9.md index 7cd818910..8ea980773 100644 --- a/docs/releases/1.17.1.9.md +++ b/docs/releases/1.17.1.9.md @@ -22,6 +22,10 @@ layout before installing an image. - Access Control replies fit the encrypted LoRa route budget and the Companion frame envelope. Large ACL listings use bounded pages. ESP32 ACL persistence avoids repeated metadata opens while retaining recovery checks. +- Companion requests skip recently used reply tags. A login reply whose + server timestamp matches the next client timestamp no longer causes an + immediate status or ACL request to fail locally. Stale replies remain + blocked, and tag selection does not change the radio's wall clock. - nRF52 keeps its 8 KiB loop stack. Crypto, contact CRC and background storage work reduce peak stack use and long pauses. OTA metadata is cached after verification instead of repeatedly reopening storage for status displays. diff --git a/examples/companion_radio/MyMesh.cpp b/examples/companion_radio/MyMesh.cpp index fe59738f5..13097a170 100644 --- a/examples/companion_radio/MyMesh.cpp +++ b/examples/companion_radio/MyMesh.cpp @@ -5130,6 +5130,20 @@ bool MyMesh::allowRequestTag(uint32_t tag) { return !_request_tag_rejected; } +bool MyMesh::allocateRequestTag(uint32_t& tag) { + // Login replies use the server's clock, so their retired tag can equal + // the next locally generated request tag even with synchronized clocks. + // Skip held tags instead of reporting BAD_STATE to the app. One history + // slot belongs to this reservation, leaving at most HISTORY_SIZE - 1 + // collisions. Keep the wall clock and all stale-response guards intact. + for (unsigned attempt = 0; + attempt < mesh::CompanionDelayedReplies::HISTORY_SIZE; ++attempt) { + tag = getRTCClock()->getCurrentTimeUnique(); + if (allowRequestTag(tag)) return true; + } + return false; +} + void MyMesh::armPendingRequest(uint32_t tag, uint32_t timeout, bool flood) { const uint32_t now = _ms->getMillis(); _delayed_replies.armRequest(tag, timeout, flood, now); diff --git a/examples/companion_radio/MyMesh.h b/examples/companion_radio/MyMesh.h index d229cde71..b416db7af 100644 --- a/examples/companion_radio/MyMesh.h +++ b/examples/companion_radio/MyMesh.h @@ -416,6 +416,7 @@ protected: uint32_t calcDirectTimeoutMillisFor(uint32_t pkt_airtime_millis, uint8_t path_len) const override; void onSendTimeout() override; bool allowRequestTag(uint32_t tag) override; + bool allocateRequestTag(uint32_t& tag) override; // DataStoreHost methods bool onContactLoaded(const ContactInfo& contact) override { return addContact(contact); } diff --git a/src/helpers/BaseChatMesh.cpp b/src/helpers/BaseChatMesh.cpp index 9af5e346e..fa6505758 100644 --- a/src/helpers/BaseChatMesh.cpp +++ b/src/helpers/BaseChatMesh.cpp @@ -814,12 +814,16 @@ int BaseChatMesh::sendLogin(const ContactInfo& recipient, const char* password, return MSG_SEND_FAILED; } +bool BaseChatMesh::allocateRequestTag(uint32_t& tag) { + tag = getRTCClock()->getCurrentTimeUnique(); + return allowRequestTag(tag); +} + int BaseChatMesh::sendAnonReq(const ContactInfo& recipient, const uint8_t* data, uint8_t len, uint32_t& tag, uint32_t& est_timeout) { mesh::Packet* pkt; { uint8_t temp[MAX_PACKET_PAYLOAD]; - tag = getRTCClock()->getCurrentTimeUnique(); - if (!allowRequestTag(tag)) return MSG_SEND_FAILED; + if (!allocateRequestTag(tag)) return MSG_SEND_FAILED; memcpy(temp, &tag, 4); // tag to match later (also extra blob to help make packet_hash unique) memcpy(&temp[4], data, len); @@ -846,8 +850,7 @@ int BaseChatMesh::sendRequest(const ContactInfo& recipient, const uint8_t* req_ mesh::Packet* pkt; { uint8_t temp[MAX_PACKET_PAYLOAD]; - tag = getRTCClock()->getCurrentTimeUnique(); - if (!allowRequestTag(tag)) return MSG_SEND_FAILED; + if (!allocateRequestTag(tag)) return MSG_SEND_FAILED; memcpy(temp, &tag, 4); // mostly an extra blob to help make packet_hash unique memcpy(&temp[4], req_data, data_len); @@ -872,8 +875,7 @@ int BaseChatMesh::sendRequest(const ContactInfo& recipient, uint8_t req_type, u mesh::Packet* pkt; { uint8_t temp[13]; - tag = getRTCClock()->getCurrentTimeUnique(); - if (!allowRequestTag(tag)) return MSG_SEND_FAILED; + if (!allocateRequestTag(tag)) return MSG_SEND_FAILED; memcpy(temp, &tag, 4); // mostly an extra blob to help make packet_hash unique temp[4] = req_type; memset(&temp[5], 0, 4); // reserved (possibly for 'since' param) diff --git a/src/helpers/BaseChatMesh.h b/src/helpers/BaseChatMesh.h index 2c61cc047..64cc0c16f 100644 --- a/src/helpers/BaseChatMesh.h +++ b/src/helpers/BaseChatMesh.h @@ -182,6 +182,7 @@ protected: virtual void onContactResponse(const ContactInfo& contact, const uint8_t* data, uint8_t len) = 0; // Reject a recently retired reflected tag before allocating/transmitting. virtual bool allowRequestTag(uint32_t tag) { return true; } + virtual bool allocateRequestTag(uint32_t& tag); virtual void handleReturnPathRetry(const ContactInfo& contact, const uint8_t* path, uint8_t path_len); virtual bool sendFloodScoped(const ContactInfo& recipient, mesh::Packet* pkt, uint32_t delay_millis=0); diff --git a/test/fixtures/companion_delayed_reply_delivery/test.cpp b/test/fixtures/companion_delayed_reply_delivery/test.cpp index 5ea2c5a04..b453732d6 100644 --- a/test/fixtures/companion_delayed_reply_delivery/test.cpp +++ b/test/fixtures/companion_delayed_reply_delivery/test.cpp @@ -87,6 +87,7 @@ struct BaseChatMesh { RtcBoundary* getRTCClock() { return &rtc; } RngBoundary* getRNG() { return &rng; } virtual bool allowRequestTag(uint32_t) { return true; } + virtual bool allocateRequestTag(uint32_t&); virtual void onContactResponse(const ContactInfo&, const uint8_t*, uint8_t) = 0; bool canMutateContacts() const { return false; } void onContactPathUpdated(ContactInfo&) {} @@ -159,6 +160,7 @@ struct MyMesh : BaseChatMesh { size_t writePendingSerialFrame(const uint8_t*, size_t, uint32_t); bool beginPendingRequest(Kind, const ContactInfo&, bool = false); bool allowRequestTag(uint32_t) override; + bool allocateRequestTag(uint32_t&) override; void armPendingRequest(uint32_t, uint32_t, bool); void finishPendingRequest(int, uint32_t, uint32_t); void abandonPendingRequest(); @@ -633,8 +635,29 @@ int main() { f.arm(Tracker::Status, 18); reply(f, Tracker::Status, nullptr, 18); f.drain(); assert(count(f.wire(), PUSH_CODE_STATUS_RESPONSE) == 1); ++checks; } - // Real Base send methods and real command branches prove rejection happens - // before packet allocation and transmission, including RTC tag re-use. + // A real login response carries the server's clock, not the client's + // request timestamp. It can equal the client's next timestamp. The app's + // immediate status/ACL request must select another tag without accepting + // the old login response or changing the wall clock. + for (bool tcp : {false, true}) for (Kind kind : { + Tracker::Status, Tracker::Binary, Tracker::Telemetry, Tracker::Discovery}) { + Fixture f(tcp); f.command(Tracker::Login); + const uint32_t server_tag = f.mesh.rtc.wall + 1; + reply(f, Tracker::Login, nullptr, server_tag); f.drain(); + const size_t request_start = f.wire().size(); + if (kind == Tracker::Binary) f.aclCommand(); else f.command(kind); + assert(f.mesh.transmissions == 2 && f.mesh.allocations == 2); + const uint32_t tag = f.mesh._delayed_replies.request.tag; + assert(tag > server_tag && f.mesh.rtc.wall == 1000); + reply(f, Tracker::Login, nullptr, server_tag); + assert(f.mesh._delayed_replies.request.phase == Tracker::AwaitRadio); + reply(f, kind, nullptr, tag); f.drain(); + assert(count(f.wire(), RESP_CODE_ERR) == 0); + assert(count(f.wire(), PUSH_CODE_LOGIN_SUCCESS) == 1); + assertSentBeforeFinal(Bytes(f.wire().begin() + request_start, f.wire().end()), + kind, tag); ++checks; + } + // An active request still rejects overlap before packet allocation/TX. for (Kind kind : {Tracker::Login, Tracker::Status, Tracker::Telemetry, Tracker::Binary, Tracker::Discovery, Tracker::Trace}) { Fixture f; f.command(kind); assert(f.mesh.transmissions == 1 && f.mesh.allocations == 1); f.command(kind); assert(f.mesh.transmissions == 1 && f.mesh.allocations == 1); ++checks; @@ -715,17 +738,45 @@ int main() { testReservedHistorySurvivesIdle(request_service, start, kind); ++checks; } } - { - Fixture f; f.command(Tracker::Binary, true); assert(f.mesh.transmissions == 1); + // RTC correction also skips recent tags. Old replies remain quarantined; + // the newly transmitted tag is the one reported to the host and matched. + for (bool anonymous : {false, true}) for (Kind kind : { + Tracker::Status, Tracker::Binary, Tracker::Telemetry, Tracker::Discovery}) { + Fixture f; f.command(kind, anonymous); + const uint32_t old_tag = f.mesh._delayed_replies.request.tag; + reply(f, kind, nullptr, old_tag); f.drain(); + const size_t request_start = f.wire().size(); + f.mesh.rtc.resetUniqueTime(old_tag); f.command(kind, anonymous); + assert(f.mesh.transmissions == 2 && f.mesh.allocations == 2); const uint32_t tag = f.mesh._delayed_replies.request.tag; - reply(f, Tracker::Binary, nullptr, tag); f.drain(); - f.mesh.rtc.resetUniqueTime(tag); f.command(Tracker::Binary, true); - assert(f.mesh.transmissions == 1 && f.mesh.allocations == 1); ++checks; + assert(tag == old_tag + 1); + reply(f, kind, nullptr, old_tag); + assert(f.mesh._delayed_replies.request.phase == Tracker::AwaitRadio); + reply(f, kind, nullptr, tag); f.drain(); + assertSentBeforeFinal(Bytes(f.wire().begin() + request_start, f.wire().end()), + kind, tag); ++checks; } - for (Kind kind : {Tracker::Status, Tracker::Binary, Tracker::Telemetry}) { - Fixture f; f.command(kind); const uint32_t tag = f.mesh._delayed_replies.request.tag; - reply(f, kind, nullptr, tag); f.drain(); f.mesh.rtc.resetUniqueTime(tag); f.command(kind); - assert(f.mesh.transmissions == 1 && f.mesh.allocations == 1); ++checks; + { + Fixture f; + for (unsigned i = 0; i < Tracker::HISTORY_SIZE - 1; ++i) { + f.command(Tracker::Binary); + reply(f, Tracker::Binary, nullptr, f.mesh._delayed_replies.request.tag); + f.drain(); + } + f.mesh.rtc.resetUniqueTime(1000); f.command(Tracker::Binary); + assert(f.mesh.transmissions == Tracker::HISTORY_SIZE); + assert(f.mesh._delayed_replies.request.tag == 1000 + Tracker::HISTORY_SIZE - 1); + assert(f.mesh.rtc.wall == 1000); ++checks; + } + { + struct RejectAllTags : MyMesh { + unsigned attempts = 0; + bool allowRequestTag(uint32_t) override { ++attempts; return false; } + } mesh; + uint32_t tag = 0; + assert(!mesh.allocateRequestTag(tag)); + assert(mesh.attempts == Tracker::HISTORY_SIZE); + assert(mesh.allocations == 0 && mesh.transmissions == 0); ++checks; } { Fixture f; diff --git a/test/fixtures/contact_cache/test_contact_cache.cpp b/test/fixtures/contact_cache/test_contact_cache.cpp index 2b2c0fab4..f35308592 100644 --- a/test/fixtures/contact_cache/test_contact_cache.cpp +++ b/test/fixtures/contact_cache/test_contact_cache.cpp @@ -193,6 +193,7 @@ public: int sendLogin(const ContactInfo&, const char*, uint32_t&); int sendAnonReq(const ContactInfo&, const uint8_t*, uint8_t, uint32_t&, uint32_t&); virtual bool allowRequestTag(uint32_t) { return true; } + virtual bool allocateRequestTag(uint32_t&); int sendRequest(const ContactInfo&, const uint8_t*, uint8_t, uint32_t&, uint32_t&); int sendRequest(const ContactInfo&, uint8_t, uint32_t&, uint32_t&); }; diff --git a/test/test_client_acl_response.py b/test/test_client_acl_response.py index 473a2ebe8..660c00430 100644 --- a/test/test_client_acl_response.py +++ b/test/test_client_acl_response.py @@ -101,7 +101,7 @@ class ClientAclResponseTest(unittest.TestCase): def test_same_clock_generates_login_and_binary_request_tags(self): source = (ROOT / "src/helpers/BaseChatMesh.cpp").read_text() for signature in ("int BaseChatMesh::sendLogin(", - "int BaseChatMesh::sendRequest(const ContactInfo& recipient, const uint8_t*"): + "bool BaseChatMesh::allocateRequestTag("): body = extract_braced(source, signature) self.assertEqual(body.count("getRTCClock()->getCurrentTimeUnique()"), 1) diff --git a/test/test_companion_delayed_reply_delivery.py b/test/test_companion_delayed_reply_delivery.py index f033b63c4..fa9d511df 100644 --- a/test/test_companion_delayed_reply_delivery.py +++ b/test/test_companion_delayed_reply_delivery.py @@ -25,7 +25,8 @@ def production_methods(source, base, packet, dispatcher): "void MyMesh::onContactResponse(", "bool MyMesh::onContactPathRecv(", "void MyMesh::onTraceRecv(", "void MyMesh::clearPendingReqs(", "bool MyMesh::hasPendingReqs(", "bool MyMesh::beginPendingRequest(", - "bool MyMesh::allowRequestTag(", "void MyMesh::armPendingRequest(", + "bool MyMesh::allowRequestTag(", "bool MyMesh::allocateRequestTag(", + "void MyMesh::armPendingRequest(", "void MyMesh::finishPendingRequest(", "void MyMesh::abandonPendingRequest(", "void MyMesh::servicePendingSerialReply(", "void MyMesh::servicePendingSerialReply(uint32_t now)", @@ -47,6 +48,7 @@ def production_methods(source, base, packet, dispatcher): methods += "\n#if COMPANION_FEATURE_TEXT_TERMINAL\n" + terminal + "\n#endif\n" methods += "\n" + "\n".join(extract_braced(base, signature) for signature in ( "bool BaseChatMesh::onContactPathRecv(", "int BaseChatMesh::sendLogin(", + "bool BaseChatMesh::allocateRequestTag(", "int BaseChatMesh::sendAnonReq(", "int BaseChatMesh::sendRequest(const ContactInfo& recipient, const uint8_t*", "int BaseChatMesh::sendRequest(const ContactInfo& recipient, uint8_t", diff --git a/test/test_contact_cache.py b/test/test_contact_cache.py index 5550b6e05..43056388e 100644 --- a/test/test_contact_cache.py +++ b/test/test_contact_cache.py @@ -108,6 +108,7 @@ class ContactCacheTest(unittest.TestCase): "namespace mesh {\n" + method(mesh, "bool Mesh::sendDirect(") + "\n}\n" + "\n".join(method(chat, signature) for signature in ( "int BaseChatMesh::sendLogin(", "int BaseChatMesh::sendAnonReq(", + "bool BaseChatMesh::allocateRequestTag(", "int BaseChatMesh::sendRequest(const ContactInfo& recipient, const uint8_t*", "int BaseChatMesh::sendRequest(const ContactInfo& recipient, uint8_t")) ) diff --git a/test/test_official_app_compatibility.py b/test/test_official_app_compatibility.py index 51a0231ae..38f748e88 100644 --- a/test/test_official_app_compatibility.py +++ b/test/test_official_app_compatibility.py @@ -140,12 +140,22 @@ int main(int argc, char** argv) { const Bytes command = fromHex(argv[1]); assert(command.size() == 40); Fixture f; f.mesh.direct_timeout = 2000; memcpy(f.mesh.recipient.id.pub_key, command.data() + 1, 32); + // Access Control is opened after login. A synchronized repeater can return + // the timestamp the Companion would otherwise choose for this app query. + f.command(Tracker::Login); + const uint32_t server_tag = f.mesh.rtc.wall + 1; + reply(f, Tracker::Login, nullptr, server_tag); f.drain(); + assert(count(f.wire(), PUSH_CODE_LOGIN_SUCCESS) == 1); + f.usb_stream.output.clear(); memcpy(f.mesh.cmd_frame, command.data(), command.size()); f.mesh.handleRequestFrame(command.size()); const auto& payload = f.mesh.transmitted_payload; assert(payload.size() == 11 && !memcmp(payload.data() + 4, command.data() + 33, 7)); uint32_t tag; memcpy(&tag, payload.data(), 4); assert(tag == slot(f, Tracker::Binary).tag && !slot(f, Tracker::Binary).sent_pending); + assert(tag > server_tag && f.mesh.rtc.wall == 1000); + reply(f, Tracker::Login, nullptr, server_tag); + assert(slot(f, Tracker::Binary).phase == Tracker::AwaitRadio); if (argc == 2) { printHex("REQUEST", payload); return 0; } Bytes radio = fromHex(argv[2]); if (atoi(argv[3]) == 0) {