diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 55b029f031..0af7ed9e05 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -368,7 +368,10 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta break; case meshtastic_AdminMessage_set_ham_mode_tag: LOG_DEBUG("Client set ham mode"); - handleSetHamMode(r->set_ham_mode); + // Without this a rejected request falls through to the generic Routing_Error_NONE ack below, + // so a client would report ham mode as enabled on a node that changed nothing. + if (!handleSetHamMode(r->set_ham_mode)) + myReply = allocErrorResponse(meshtastic_Routing_Error_BAD_REQUEST, &mp); break; case meshtastic_AdminMessage_get_ui_config_request_tag: { LOG_DEBUG("Client is getting device-ui config"); @@ -1920,30 +1923,40 @@ void AdminModule::handleStoreDeviceUIConfig(const meshtastic_DeviceUIConfig &uic #endif } -void AdminModule::handleSetHamMode(const meshtastic_HamParameters &p) +// Unset, or set to nothing but whitespace - the two ways a client can leave a name field empty. +static bool isBlankName(const char *start) { - // Validate ham parameters before setting since this would bypass validation in the owner struct - const char *fieldsToCheck[] = {p.call_sign, p.short_name}; - const char *fieldNames[] = {"call_sign", "short_name"}; - for (int i = 0; i < 2; i++) { - if (*fieldsToCheck[i]) { - const char *start = fieldsToCheck[i]; - while (*start && isspace((unsigned char)*start)) - start++; - if (*start == '\0') { - LOG_WARN("Rejected ham %s: needs 1+ non-whitespace char", fieldNames[i]); - return; - } - } + while (*start && isspace((unsigned char)*start)) + start++; + return *start == '\0'; +} + +bool AdminModule::handleSetHamMode(const meshtastic_HamParameters &p) +{ + // Validate ham parameters before setting since this would bypass validation in the owner struct. + + // The call sign is the station ID the whole licensed mode is built around, so it is required; + // without it we would license a node that never identifies itself on the air. + if (isBlankName(p.call_sign)) { + LOG_WARN("Rejected ham call_sign: needs 1+ non-whitespace char"); + return false; } - // Set call sign and override lora limitations for licensed use - strncpy(owner.long_name, p.call_sign, sizeof(owner.long_name)); + // Set call sign and override lora limitations for licensed use. An optional long_name rides + // behind the call sign with the "//" separator hams already use on the air. + // e.g. call_sign "N0CALL" plus long_name "Attic Heltec" becomes "N0CALL//Attic Heltec". + if (!isBlankName(p.long_name)) + snprintf(owner.long_name, sizeof(owner.long_name), "%s//%s", p.call_sign, p.long_name); + else + strncpy(owner.long_name, p.call_sign, sizeof(owner.long_name)); owner.long_name[sizeof(owner.long_name) - 1] = '\0'; - sanitizeUtf8(owner.long_name, sizeof(owner.long_name)); - strncpy(owner.short_name, p.short_name, sizeof(owner.short_name)); - owner.short_name[sizeof(owner.short_name) - 1] = '\0'; - sanitizeUtf8(owner.short_name, sizeof(owner.short_name)); + clampLongName(owner.long_name); + // short_name is optional per the schema, so a blank one keeps the name the node already had + if (!isBlankName(p.short_name)) { + strncpy(owner.short_name, p.short_name, sizeof(owner.short_name)); + owner.short_name[sizeof(owner.short_name) - 1] = '\0'; + sanitizeUtf8(owner.short_name, sizeof(owner.short_name)); + } owner.is_licensed = true; config.lora.override_duty_cycle = true; config.lora.tx_power = p.tx_power; @@ -1977,6 +1990,7 @@ void AdminModule::handleSetHamMode(const meshtastic_HamParameters &p) service->reloadOwner(false); saveChanges(SEGMENT_CONFIG | SEGMENT_NODEDATABASE | SEGMENT_DEVICESTATE | SEGMENT_CHANNELS); + return true; } AdminModule::AdminModule() : ProtobufModule("Admin", meshtastic_PortNum_ADMIN_APP, &meshtastic_AdminMessage_msg) diff --git a/src/modules/AdminModule.h b/src/modules/AdminModule.h index b8e8c59ade..0fbc8ac3b1 100644 --- a/src/modules/AdminModule.h +++ b/src/modules/AdminModule.h @@ -95,7 +95,9 @@ class AdminModule : public ProtobufModule, public Obser void handleSetChannel(); public: - void handleSetHamMode(const meshtastic_HamParameters &req); + /// Applies licensed-operator settings. False if the request was rejected and nothing changed, + /// so the caller can answer a want_response client with an error instead of an implicit success. + bool handleSetHamMode(const meshtastic_HamParameters &req); /// Note an admin request leaving this node for a remote, so that remote's response is /// accepted. Called from the client-to-mesh path (MeshService::handleToRadio). diff --git a/test/test_admin_radio/test_main.cpp b/test/test_admin_radio/test_main.cpp index 004037c9ad..8faebd16a6 100644 --- a/test/test_admin_radio/test_main.cpp +++ b/test/test_admin_radio/test_main.cpp @@ -21,6 +21,7 @@ #include "TestUtil.h" #include "graphics/draw/MenuHandler.h" #include "mesh/Channels.h" +#include "mesh/Router.h" // router global: allocErrorResponse() allocates the reply through it #include "modules/AdminModule.h" #include "modules/NodeInfoModule.h" #include // crc32Buffer(), for the my_node_num == crc32(public_key) invariant @@ -1000,6 +1001,10 @@ static meshtastic_DeviceState savedDeviceState; static meshtastic_User savedOwner; static meshtastic_LocalConfig savedConfig; static meshtastic_ChannelFile savedChannelFile; +// Only the ham dispatcher test installs a router (allocErrorResponse() allocates through it). +// Saved/torn down for every test so a failed assertion's longjmp cannot leave one dangling. +static Router *savedRouter; +static Router *hamMockRouter; // Called from setUp/tearDown for every test, not opted into by a handful. A shared NodeDB plus // unrestored config/owner/devicestate/channelFile means each test inherits whatever its @@ -1008,6 +1013,7 @@ static void replaceAdminRadioGlobals() { savedNodeDB = nodeDB; savedNodeInfoModule = nodeInfoModule; + savedRouter = router; savedDeviceState = devicestate; savedOwner = owner; savedConfig = config; @@ -1020,6 +1026,9 @@ static void restoreAdminRadioGlobals() { nodeInfoModule = savedNodeInfoModule; nodeDB = savedNodeDB; + router = savedRouter; + delete hamMockRouter; + hamMockRouter = nullptr; delete replacementNodeDB; replacementNodeDB = nullptr; devicestate = savedDeviceState; @@ -1084,6 +1093,210 @@ static void test_handleSetOwner_persistsLicensedChannelSanitation() TEST_ASSERT_FALSE_MESSAGE(channels.ensureLicensedOperation(), "sanitized reload must not trigger another persistence write"); } +// ----------------------------------------------------------------------- +// handleSetHamMode() name assembly: the ham long_name rides behind the call +// sign with the "//" separator hams already use on the air. +// ----------------------------------------------------------------------- + +// Licensing a node touches channels, the NodeDB and the owner struct; an UNSET region keeps the +// keygen/identity-migration path out of these name-only assertions. +static void primeHamModeTest() +{ + owner = meshtastic_User_init_zero; + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_UNSET; + channels.initDefaults(); + nodeInfoModule = reinterpret_cast(1); // reloadOwner(false) only checks presence + testAdmin->deferSaves(); +} + +static void test_handleSetHamMode_appendsLongNameToCallSign() +{ + primeHamModeTest(); + + meshtastic_HamParameters p = meshtastic_HamParameters_init_zero; + strncpy(p.call_sign, "KD2ABC", sizeof(p.call_sign) - 1); + strncpy(p.short_name, "ABC", sizeof(p.short_name) - 1); + strncpy(p.long_name, "Attic Heltec", sizeof(p.long_name) - 1); + TEST_ASSERT_TRUE(testAdmin->handleSetHamMode(p)); + + TEST_ASSERT_EQUAL_STRING("KD2ABC//Attic Heltec", owner.long_name); + TEST_ASSERT_EQUAL_STRING("ABC", owner.short_name); + TEST_ASSERT_TRUE(owner.is_licensed); +} + +// The widest pair the proto can carry (7 + 2 + 14) still has to arrive whole, or the operator +// silently loses the tail of the name they typed. +static void test_handleSetHamMode_widestPairSurvivesTheLongNameCap() +{ + primeHamModeTest(); + + meshtastic_HamParameters p = meshtastic_HamParameters_init_zero; + strncpy(p.call_sign, "KD2ABCD", sizeof(p.call_sign) - 1); + strncpy(p.long_name, "Attic Heltec 3", sizeof(p.long_name) - 1); + TEST_ASSERT_TRUE(testAdmin->handleSetHamMode(p)); + + TEST_ASSERT_EQUAL_STRING("KD2ABCD//Attic Heltec 3", owner.long_name); + TEST_ASSERT_LESS_OR_EQUAL(MAX_LONG_NAME_BYTES, strlen(owner.long_name)); +} + +static void test_handleSetHamMode_omittedLongNameKeepsCallSignAlone() +{ + primeHamModeTest(); + + meshtastic_HamParameters p = meshtastic_HamParameters_init_zero; + strncpy(p.call_sign, "KD2ABC", sizeof(p.call_sign) - 1); + testAdmin->handleSetHamMode(p); + + TEST_ASSERT_EQUAL_STRING("KD2ABC", owner.long_name); + TEST_ASSERT_TRUE(owner.is_licensed); +} + +// long_name is optional both ways a client can leave it empty: a whitespace-only one is dropped +// (no dangling "//" on the air) instead of costing the operator the whole licensing request. +static void test_handleSetHamMode_blankLongNameIsIgnoredNotRejected() +{ + primeHamModeTest(); + + meshtastic_HamParameters p = meshtastic_HamParameters_init_zero; + strncpy(p.call_sign, "KD2ABC", sizeof(p.call_sign) - 1); + strncpy(p.long_name, " ", sizeof(p.long_name) - 1); + testAdmin->handleSetHamMode(p); + + TEST_ASSERT_EQUAL_STRING("KD2ABC", owner.long_name); + TEST_ASSERT_TRUE(owner.is_licensed); +} + +// The call sign is required, unlike the two optional name fields: an empty one would license a +// node that never identifies itself, and once a long_name is set it would compose to a dangling +// "//Attic Heltec". +static void test_handleSetHamMode_blankCallSignIsRejected() +{ + primeHamModeTest(); + + meshtastic_HamParameters missing = meshtastic_HamParameters_init_zero; + strncpy(missing.long_name, "Attic Heltec", sizeof(missing.long_name) - 1); + TEST_ASSERT_FALSE(testAdmin->handleSetHamMode(missing)); + + TEST_ASSERT_EQUAL_STRING("", owner.long_name); + TEST_ASSERT_FALSE(owner.is_licensed); + + primeHamModeTest(); + + meshtastic_HamParameters whitespace = meshtastic_HamParameters_init_zero; + strncpy(whitespace.call_sign, " ", sizeof(whitespace.call_sign) - 1); + TEST_ASSERT_FALSE(testAdmin->handleSetHamMode(whitespace)); + + TEST_ASSERT_EQUAL_STRING("", owner.long_name); + TEST_ASSERT_FALSE(owner.is_licensed); +} + +// short_name is optional too, so a blank one keeps whatever the node was already called instead of +// blanking it - licensing the node must not cost the operator their existing short name. +static void test_handleSetHamMode_blankShortNameKeepsTheExistingOne() +{ + for (const char *blank : {"", " "}) { + primeHamModeTest(); + strncpy(owner.short_name, "OLD", sizeof(owner.short_name) - 1); + + meshtastic_HamParameters p = meshtastic_HamParameters_init_zero; + strncpy(p.call_sign, "KD2ABC", sizeof(p.call_sign) - 1); + strncpy(p.short_name, blank, sizeof(p.short_name) - 1); + TEST_ASSERT_TRUE(testAdmin->handleSetHamMode(p)); + + TEST_ASSERT_EQUAL_STRING("OLD", owner.short_name); + TEST_ASSERT_EQUAL_STRING("KD2ABC", owner.long_name); + TEST_ASSERT_TRUE(owner.is_licensed); + } +} + +// A rejection has to reach the client, not just the log: allocErrorResponse() builds the reply +// through the router, so this is the one ham test that needs one. +class HamModeMockRouter : public Router +{ + public: + ~HamModeMockRouter() + { + delete cryptLock; // the Router ctor asserts this is clear, so a later suite can construct one + cryptLock = nullptr; + } + ErrorCode send(meshtastic_MeshPacket *p) override + { + packetPool.release(p); + return ERRNO_OK; + } +}; + +// Pull the Routing error out of the ack/nak a handler queued in myReply. +static bool decodeRoutingError(meshtastic_MeshPacket *reply, meshtastic_Routing_Error &out) +{ + if (!reply || reply->which_payload_variant != meshtastic_MeshPacket_decoded_tag) + return false; + if (reply->decoded.portnum != meshtastic_PortNum_ROUTING_APP) + return false; + meshtastic_Routing routing = meshtastic_Routing_init_zero; + if (!pb_decode_from_bytes(reply->decoded.payload.bytes, reply->decoded.payload.size, &meshtastic_Routing_msg, &routing)) + return false; + if (routing.which_variant != meshtastic_Routing_error_reason_tag) + return false; + out = routing.error_reason; + return true; +} + +// Handler-level rejection is invisible to a want_response client on its own: with no reply queued, +// handleReceivedProtobuf() falls through to its generic "ACK" and answers Routing_Error_NONE, so the +// app reports ham mode as enabled on a node that changed nothing. The dispatcher has to say +// BAD_REQUEST before that fallback runs. +static void test_handleSetHamMode_blankCallSignRepliesBadRequest() +{ + primeHamModeTest(); + hamMockRouter = new HamModeMockRouter(); + router = hamMockRouter; + + meshtastic_AdminMessage m = meshtastic_AdminMessage_init_zero; + m.which_payload_variant = meshtastic_AdminMessage_set_ham_mode_tag; + strncpy(m.set_ham_mode.long_name, "Attic Heltec", sizeof(m.set_ham_mode.long_name) - 1); + + meshtastic_MeshPacket mp = meshtastic_MeshPacket_init_zero; + mp.from = 0; // local client, so the passkey gate is bypassed and the switch body runs + mp.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + mp.decoded.want_response = true; + testAdmin->handleReceivedProtobuf(mp, &m); + + meshtastic_Routing_Error err = meshtastic_Routing_Error_NONE; + TEST_ASSERT_TRUE_MESSAGE(decodeRoutingError(testAdmin->reply(), err), "a rejected request must queue an error reply"); + TEST_ASSERT_EQUAL(meshtastic_Routing_Error_BAD_REQUEST, err); + TEST_ASSERT_FALSE(owner.is_licensed); + testAdmin->drainReply(); +} + +// The other half of the pair: an accepted request still answers Routing_Error_NONE. Asserting both +// sides is the point - NONE is what the rejection path used to borrow, so a test that only checked +// the reject case could pass against a handler that answered NONE to everything. +static void test_handleSetHamMode_acceptedRequestAcksSuccess() +{ + primeHamModeTest(); + hamMockRouter = new HamModeMockRouter(); + router = hamMockRouter; + + meshtastic_AdminMessage m = meshtastic_AdminMessage_init_zero; + m.which_payload_variant = meshtastic_AdminMessage_set_ham_mode_tag; + strncpy(m.set_ham_mode.call_sign, "KD2ABC", sizeof(m.set_ham_mode.call_sign) - 1); + strncpy(m.set_ham_mode.long_name, "Attic Heltec", sizeof(m.set_ham_mode.long_name) - 1); + + meshtastic_MeshPacket mp = meshtastic_MeshPacket_init_zero; + mp.from = 0; + mp.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + mp.decoded.want_response = true; + testAdmin->handleReceivedProtobuf(mp, &m); + + meshtastic_Routing_Error err = meshtastic_Routing_Error_BAD_REQUEST; + TEST_ASSERT_TRUE_MESSAGE(decodeRoutingError(testAdmin->reply(), err), "want_response must be answered"); + TEST_ASSERT_EQUAL(meshtastic_Routing_Error_NONE, err); + TEST_ASSERT_EQUAL_STRING("KD2ABC//Attic Heltec", owner.long_name); + TEST_ASSERT_TRUE(owner.is_licensed); + testAdmin->drainReply(); +} + static void test_bootDefense_sanitizesStaleLicensedChannelsOnce() { owner = meshtastic_User_init_zero; @@ -2001,6 +2214,14 @@ void setup() // getRegion() RUN_TEST(test_handleSetOwner_persistsLicensedChannelSanitation); + RUN_TEST(test_handleSetHamMode_appendsLongNameToCallSign); + RUN_TEST(test_handleSetHamMode_widestPairSurvivesTheLongNameCap); + RUN_TEST(test_handleSetHamMode_omittedLongNameKeepsCallSignAlone); + RUN_TEST(test_handleSetHamMode_blankLongNameIsIgnoredNotRejected); + RUN_TEST(test_handleSetHamMode_blankShortNameKeepsTheExistingOne); + RUN_TEST(test_handleSetHamMode_blankCallSignIsRejected); + RUN_TEST(test_handleSetHamMode_blankCallSignRepliesBadRequest); + RUN_TEST(test_handleSetHamMode_acceptedRequestAcksSuccess); RUN_TEST(test_handleSetConfig_persistsLicensedFirstRegionIdentity); RUN_TEST(test_handleSetConfig_persistsUnlicensedFirstRegionIdentity); RUN_TEST(test_bootDefense_sanitizesStaleLicensedChannelsOnce);