From 03e6b80989b364efd479c2778424c1d459277eaf Mon Sep 17 00:00:00 2001 From: Tom <116762865+NomDeTom@users.noreply.github.com> Date: Wed, 5 Aug 2026 07:55:31 +0100 Subject: [PATCH] Serial config validation (#11339) * fix(serial): validate serial module config on every platform AdminModule guarded the serial config validation by architecture but not the assignment beneath it: #if ARCH_ESP32 || ARCH_NRF52 || ARCH_RP2040 if (!SerialModule::isValidConfig(...)) return false; disableBluetooth(); #endif moduleConfig.serial = c.payload_variant.serial; So on every other platform an admin "set module config: serial" stored a config the firmware rejects on ESP32. override_console_serial_port combined with DEFAULT, SIMPLE, TEXTMSG or PROTO is accepted and persisted today. Two families are affected, for different reasons: - portduino/meshtasticd, where the validation did not exist at all: isValidConfig was a static member of SerialModule, and that class is inside the same architecture guard, so `nm` finds no such symbol in the native object. - STM32WL (rak3172, wio-e5, CDEBYTE_E77-MBL, russell), where it existed and was never called: the class guard includes ARCH_STM32WL and the AdminModule call site did not. Validation is pure config logic with no serial hardware behind it, so it moves out of the class and out of the guard as a free serialConfigIsValid(). Its only external references - clientNotificationPool, service, getValidTime - are already unguarded elsewhere, so it links on every target. AdminModule's include of SerialModule.h is unguarded for the same reason; the class itself stays guarded inside the header. Only disableBluetooth() remains architecture-specific. This changes what meshtasticd and the STM32WL targets accept: a host relying on the unvalidated path (override_console_serial_port with a mode other than NMEA, CalTopo or MS_CONFIG) is now rejected, as it already is on ESP32. test/test_serial has asserted nothing since it was added in 28aeb0f09e (2025-07-26): its body is behind the same guard, so on portduino it logged a warning and ran zero assertions while counting as one of the canonical suites. Enabling it showed the code did not even compile - its designated initializers list .override_console_serial_port before .mode, which is not declaration order, and C++ requires that. PlatformIO only builds test/ for the native env, so no build had ever compiled these lines. Reordered; all nine now run and pass. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * I'd say gimme 5 bees for a dollar. That's what we called a nickel, because they had bees on em. * style: wrap over-long warning string to the 120-col limit * Cover MS_CONFIG override and correct the validator comment serialConfigIsValid() accepts MS_CONFIG alongside NMEA and CALTOPO when override_console_serial_port is set, but only the first two had a valid-case test. Add the missing one. The declaration comment described the function as pure config logic; it also logs and, in non-test builds, sends a client notification on rejection. --------- Co-authored-by: Ben Meadors Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/modules/AdminModule.cpp | 11 ++--- src/modules/SerialModule.cpp | 47 ++++++++++----------- src/modules/SerialModule.h | 7 +++- test/test_serial/SerialModule.cpp | 68 +++++++++++++++---------------- 4 files changed, 65 insertions(+), 68 deletions(-) diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 98a9c9d93..410ce8fed 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -63,10 +63,8 @@ #if !defined(ARCH_STM32WL) && !MESHTASTIC_EXCLUDE_I2C && !MESHTASTIC_EXCLUDE_ACCELEROMETER #include "motion/AccelerometerThread.h" #endif -#if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040)) && !defined(CONFIG_IDF_TARGET_ESP32S2) && \ - !defined(CONFIG_IDF_TARGET_ESP32C3) +// Unguarded: serialConfigIsValid() is needed on every platform. The class stays guarded in the header. #include "SerialModule.h" -#endif AdminModule *adminModule; @@ -1271,14 +1269,13 @@ bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c) break; case meshtastic_ModuleConfig_serial_tag: LOG_INFO("Set module config: Serial"); -#if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040)) && !defined(CONFIG_IDF_TARGET_ESP32S2) && \ - !defined(CONFIG_IDF_TARGET_ESP32C3) - if (!SerialModule::isValidConfig(c.payload_variant.serial)) { + // No architecture guard: the check and the store below must agree on every platform. + // disableBluetooth() self-guards on HAS_BLUETOOTH, so it is empty where there is no radio. + if (!serialConfigIsValid(c.payload_variant.serial)) { LOG_ERROR("Invalid serial config"); return false; } disableBluetooth(); // Disable Bluetooth to prevent interference during Serial configuration -#endif moduleConfig.has_serial = true; moduleConfig.serial = c.payload_variant.serial; break; diff --git a/src/modules/SerialModule.cpp b/src/modules/SerialModule.cpp index cb481e6a5..ef26bc360 100644 --- a/src/modules/SerialModule.cpp +++ b/src/modules/SerialModule.cpp @@ -49,6 +49,30 @@ #include "meshSolarApp.h" #endif +// Outside the architecture guard on purpose: config validation, not serial I/O. See SerialModule.h. +bool serialConfigIsValid(const meshtastic_ModuleConfig_SerialConfig &config) +{ + if (config.override_console_serial_port && !IS_ONE_OF(config.mode, meshtastic_ModuleConfig_SerialConfig_Serial_Mode_NMEA, + meshtastic_ModuleConfig_SerialConfig_Serial_Mode_CALTOPO, + meshtastic_ModuleConfig_SerialConfig_Serial_Mode_MS_CONFIG)) { + const char *warning = "Invalid Serial config: override console serial port is only supported in NMEA, CalTopo, or MS " + "Config output-only modes."; + LOG_ERROR(warning); +#ifndef PIO_UNIT_TESTING + meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed(); + if (cn) { + cn->level = meshtastic_LogRecord_Level_ERROR; + cn->time = getValidTime(RTCQualityFromNet); + snprintf(cn->message, sizeof(cn->message), "%s", warning); + service->sendClientNotification(cn); + } +#endif + return false; + } + + return true; +} + #if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040) || defined(ARCH_STM32WL)) && \ !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) @@ -86,29 +110,6 @@ static Print *serialPrint = &SERIAL_PRINT_OBJECT; char serialBytes[512]; size_t serialPayloadSize; -bool SerialModule::isValidConfig(const meshtastic_ModuleConfig_SerialConfig &config) -{ - if (config.override_console_serial_port && !IS_ONE_OF(config.mode, meshtastic_ModuleConfig_SerialConfig_Serial_Mode_NMEA, - meshtastic_ModuleConfig_SerialConfig_Serial_Mode_CALTOPO, - meshtastic_ModuleConfig_SerialConfig_Serial_Mode_MS_CONFIG)) { - const char *warning = - "Invalid Serial config: override console serial port is only supported in NMEA and CalTopo output-only modes."; - LOG_ERROR(warning); -#ifndef PIO_UNIT_TESTING - meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed(); - if (cn) { - cn->level = meshtastic_LogRecord_Level_ERROR; - cn->time = getValidTime(RTCQualityFromNet); - snprintf(cn->message, sizeof(cn->message), "%s", warning); - service->sendClientNotification(cn); - } -#endif - return false; - } - - return true; -} - SerialModuleRadio::SerialModuleRadio() : SinglePortModule("SerialModuleRadio", meshtastic_PortNum_SERIAL_APP) { switch (moduleConfig.serial.mode) { diff --git a/src/modules/SerialModule.h b/src/modules/SerialModule.h index 5cbca7824..8121486e6 100644 --- a/src/modules/SerialModule.h +++ b/src/modules/SerialModule.h @@ -8,6 +8,11 @@ #include #include +// Is this serial config one we will accept? Outside the architecture guard below because it touches +// no serial hardware, and AdminModule must run it on every platform - including those where +// SerialModule itself does not exist. Logs and notifies the client on rejection. +bool serialConfigIsValid(const meshtastic_ModuleConfig_SerialConfig &config); + #if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040) || defined(ARCH_STM32WL)) && \ !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) @@ -20,8 +25,6 @@ class SerialModule : public StreamAPI, private concurrency::OSThread public: SerialModule(); - static bool isValidConfig(const meshtastic_ModuleConfig_SerialConfig &config); - protected: virtual int32_t runOnce() override; diff --git a/test/test_serial/SerialModule.cpp b/test/test_serial/SerialModule.cpp index 39992b3a2..6539d0ad3 100644 --- a/test/test_serial/SerialModule.cpp +++ b/test/test_serial/SerialModule.cpp @@ -5,20 +5,14 @@ #ifdef ARCH_PORTDUINO #include "configuration.h" -#if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040)) && !defined(CONFIG_IDF_TARGET_ESP32S2) && \ - !defined(CONFIG_IDF_TARGET_ESP32C3) #include "modules/SerialModule.h" -#endif - -#if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040)) && !defined(CONFIG_IDF_TARGET_ESP32S2) && \ - !defined(CONFIG_IDF_TARGET_ESP32C3) // Test that empty configuration is valid. void test_serialConfigEmptyIsValid(void) { meshtastic_ModuleConfig_SerialConfig config = {}; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); } // Test that basic enabled configuration is valid. @@ -26,61 +20,71 @@ void test_serialConfigEnabledIsValid(void) { meshtastic_ModuleConfig_SerialConfig config = {.enabled = true}; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); } // Test that configuration with override_console_serial_port and NMEA mode is valid. void test_serialConfigWithOverrideConsoleNmeaModeIsValid(void) { meshtastic_ModuleConfig_SerialConfig config = { - .enabled = true, .override_console_serial_port = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_NMEA}; + .enabled = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_NMEA, .override_console_serial_port = true}; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); } // Test that configuration with override_console_serial_port and CalTopo mode is valid. void test_serialConfigWithOverrideConsoleCalTopoModeIsValid(void) { meshtastic_ModuleConfig_SerialConfig config = { - .enabled = true, .override_console_serial_port = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_CALTOPO}; + .enabled = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_CALTOPO, .override_console_serial_port = true}; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); +} + +// Test that configuration with override_console_serial_port and MS Config mode is valid. +void test_serialConfigWithOverrideConsoleMsConfigModeIsValid(void) +{ + meshtastic_ModuleConfig_SerialConfig config = {.enabled = true, + .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_MS_CONFIG, + .override_console_serial_port = true}; + + TEST_ASSERT_TRUE(serialConfigIsValid(config)); } // Test that configuration with override_console_serial_port and DEFAULT mode is invalid. void test_serialConfigWithOverrideConsoleDefaultModeIsInvalid(void) { meshtastic_ModuleConfig_SerialConfig config = { - .enabled = true, .override_console_serial_port = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_DEFAULT}; + .enabled = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_DEFAULT, .override_console_serial_port = true}; - TEST_ASSERT_FALSE(SerialModule::isValidConfig(config)); + TEST_ASSERT_FALSE(serialConfigIsValid(config)); } // Test that configuration with override_console_serial_port and SIMPLE mode is invalid. void test_serialConfigWithOverrideConsoleSimpleModeIsInvalid(void) { meshtastic_ModuleConfig_SerialConfig config = { - .enabled = true, .override_console_serial_port = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_SIMPLE}; + .enabled = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_SIMPLE, .override_console_serial_port = true}; - TEST_ASSERT_FALSE(SerialModule::isValidConfig(config)); + TEST_ASSERT_FALSE(serialConfigIsValid(config)); } // Test that configuration with override_console_serial_port and TEXTMSG mode is invalid. void test_serialConfigWithOverrideConsoleTextMsgModeIsInvalid(void) { meshtastic_ModuleConfig_SerialConfig config = { - .enabled = true, .override_console_serial_port = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_TEXTMSG}; + .enabled = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_TEXTMSG, .override_console_serial_port = true}; - TEST_ASSERT_FALSE(SerialModule::isValidConfig(config)); + TEST_ASSERT_FALSE(serialConfigIsValid(config)); } // Test that configuration with override_console_serial_port and PROTO mode is invalid. void test_serialConfigWithOverrideConsoleProtoModeIsInvalid(void) { meshtastic_ModuleConfig_SerialConfig config = { - .enabled = true, .override_console_serial_port = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_PROTO}; + .enabled = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_PROTO, .override_console_serial_port = true}; - TEST_ASSERT_FALSE(SerialModule::isValidConfig(config)); + TEST_ASSERT_FALSE(serialConfigIsValid(config)); } // Test that various modes work without override_console_serial_port. @@ -90,53 +94,45 @@ void test_serialConfigVariousModesWithoutOverrideAreValid(void) // Test DEFAULT mode config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_DEFAULT; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); // Test SIMPLE mode config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_SIMPLE; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); // Test TEXTMSG mode config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_TEXTMSG; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); // Test PROTO mode config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_PROTO; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); // Test NMEA mode config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_NMEA; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); // Test CALTOPO mode config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_CALTOPO; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); } -#endif // Architecture check - void setup() { initializeTestEnvironment(); -#if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040)) && !defined(CONFIG_IDF_TARGET_ESP32S2) && \ - !defined(CONFIG_IDF_TARGET_ESP32C3) UNITY_BEGIN(); RUN_TEST(test_serialConfigEmptyIsValid); RUN_TEST(test_serialConfigEnabledIsValid); RUN_TEST(test_serialConfigWithOverrideConsoleNmeaModeIsValid); RUN_TEST(test_serialConfigWithOverrideConsoleCalTopoModeIsValid); + RUN_TEST(test_serialConfigWithOverrideConsoleMsConfigModeIsValid); RUN_TEST(test_serialConfigWithOverrideConsoleDefaultModeIsInvalid); RUN_TEST(test_serialConfigWithOverrideConsoleSimpleModeIsInvalid); RUN_TEST(test_serialConfigWithOverrideConsoleTextMsgModeIsInvalid); RUN_TEST(test_serialConfigWithOverrideConsoleProtoModeIsInvalid); RUN_TEST(test_serialConfigVariousModesWithoutOverrideAreValid); exit(UNITY_END()); -#else - LOG_WARN("This test requires ESP32, NRF52, or RP2040 architecture"); - UNITY_BEGIN(); - UNITY_END(); -#endif } #else void setup() @@ -144,7 +140,7 @@ void setup() initializeTestEnvironment(); LOG_WARN("This test requires the ARCH_PORTDUINO variant"); UNITY_BEGIN(); - UNITY_END(); + exit(UNITY_END()); } #endif void loop() {}