Validate Serial config console override modes (#7470)

* Validate serial config console override modes

* Update src/modules/SerialModule.cpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Disable

* Guard serial module

* Guards

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

src/modules/AdminModule.cpp

@@ -43,6 +43,10 @@
 #if !defined(ARCH_STM32WL) && !MESHTASTIC_EXCLUDE_I2C
 #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)
+#include "SerialModule.h"
+#endif
 
 AdminModule *adminModule;
 bool hasOpenEditTransaction;
@@ -807,10 +811,12 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c)
 
 bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c)
 {
-    // If we are in an open transaction or configuring MQTT, defer disabling Bluetooth
+    // If we are in an open transaction or configuring MQTT or Serial (which have validation), defer disabling Bluetooth
     // Otherwise, disable Bluetooth to prevent the phone from interfering with the config
-    if (!hasOpenEditTransaction && c.which_payload_variant != meshtastic_ModuleConfig_mqtt_tag)
+    if (!hasOpenEditTransaction &&
+        !IS_ONE_OF(c.which_payload_variant, meshtastic_ModuleConfig_mqtt_tag, meshtastic_ModuleConfig_serial_tag)) {
         disableBluetooth();
+    }
 
     switch (c.which_payload_variant) {
     case meshtastic_ModuleConfig_mqtt_tag:
@@ -830,6 +836,14 @@ 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)) {
+            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;
@@ -985,9 +999,10 @@ void AdminModule::handleGetConfig(const meshtastic_MeshPacket &req, const uint32
         // So even if we internally use 0 to represent 'use default' we still need to send the value we are
         // using to the app (so that even old phone apps work with new device loads).
         // r.get_radio_response.preferences.ls_secs = getPref_ls_secs();
-        // hideSecret(r.get_radio_response.preferences.wifi_ssid); // hmm - leave public for now, because only minimally private
-        // and useful for users to know current provisioning) hideSecret(r.get_radio_response.preferences.wifi_password);
-        // r.get_config_response.which_payloadVariant = Config_ModuleConfig_telemetry_tag;
+        // hideSecret(r.get_radio_response.preferences.wifi_ssid); // hmm - leave public for now, because only minimally
+        // private and useful for users to know current provisioning)
+        // hideSecret(r.get_radio_response.preferences.wifi_password); r.get_config_response.which_payloadVariant =
+        // Config_ModuleConfig_telemetry_tag;
         res.which_payload_variant = meshtastic_AdminMessage_get_config_response_tag;
         setPassKey(&res);
         myReply = allocDataProtobuf(res);
@@ -1071,9 +1086,10 @@ void AdminModule::handleGetModuleConfig(const meshtastic_MeshPacket &req, const
         // So even if we internally use 0 to represent 'use default' we still need to send the value we are
         // using to the app (so that even old phone apps work with new device loads).
         // r.get_radio_response.preferences.ls_secs = getPref_ls_secs();
-        // hideSecret(r.get_radio_response.preferences.wifi_ssid); // hmm - leave public for now, because only minimally private
-        // and useful for users to know current provisioning) hideSecret(r.get_radio_response.preferences.wifi_password);
-        // r.get_config_response.which_payloadVariant = Config_ModuleConfig_telemetry_tag;
+        // hideSecret(r.get_radio_response.preferences.wifi_ssid); // hmm - leave public for now, because only minimally
+        // private and useful for users to know current provisioning)
+        // hideSecret(r.get_radio_response.preferences.wifi_password); r.get_config_response.which_payloadVariant =
+        // Config_ModuleConfig_telemetry_tag;
         res.which_payload_variant = meshtastic_AdminMessage_get_module_config_response_tag;
         setPassKey(&res);
         myReply = allocDataProtobuf(res);

src/modules/SerialModule.cpp

@@ -74,6 +74,26 @@ static Print *serialPrint = &Serial2;
 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)) {
+        const char *warning =
+            "Invalid Serial config: override console serial port is only supported in NMEA and CalTopo output-only modes.";
+        LOG_ERROR(warning);
+#if !IS_RUNNING_TESTS
+        meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed();
+        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() : MeshModule("SerialModuleRadio")
 {
     switch (moduleConfig.serial.mode) {

src/modules/SerialModule.h

@@ -20,6 +20,8 @@ class SerialModule : public StreamAPI, private concurrency::OSThread
   public:
     SerialModule();
 
+    static bool isValidConfig(const meshtastic_ModuleConfig_SerialConfig &config);
+
   protected:
     virtual int32_t runOnce() override;
 

test/test_serial/SerialModule.cpp

@@ -0,0 +1,156 @@
+#include "DebugConfiguration.h"
+#include "TestUtil.h"
+#include <unity.h>
+
+#ifdef ARCH_PORTDUINO
+#include "configuration.h"
+
+#if defined(UNIT_TEST)
+#define IS_RUNNING_TESTS 1
+#else
+#define IS_RUNNING_TESTS 0
+#endif
+
+#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 that basic enabled configuration is valid.
+void test_serialConfigEnabledIsValid(void)
+{
+    meshtastic_ModuleConfig_SerialConfig config = {.enabled = true};
+
+    TEST_ASSERT_TRUE(SerialModule::isValidConfig(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};
+
+    TEST_ASSERT_TRUE(SerialModule::isValidConfig(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};
+
+    TEST_ASSERT_TRUE(SerialModule::isValidConfig(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};
+
+    TEST_ASSERT_FALSE(SerialModule::isValidConfig(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};
+
+    TEST_ASSERT_FALSE(SerialModule::isValidConfig(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};
+
+    TEST_ASSERT_FALSE(SerialModule::isValidConfig(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};
+
+    TEST_ASSERT_FALSE(SerialModule::isValidConfig(config));
+}
+
+// Test that various modes work without override_console_serial_port.
+void test_serialConfigVariousModesWithoutOverrideAreValid(void)
+{
+    meshtastic_ModuleConfig_SerialConfig config = {.enabled = true, .override_console_serial_port = false};
+
+    // Test DEFAULT mode
+    config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_DEFAULT;
+    TEST_ASSERT_TRUE(SerialModule::isValidConfig(config));
+
+    // Test SIMPLE mode
+    config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_SIMPLE;
+    TEST_ASSERT_TRUE(SerialModule::isValidConfig(config));
+
+    // Test TEXTMSG mode
+    config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_TEXTMSG;
+    TEST_ASSERT_TRUE(SerialModule::isValidConfig(config));
+
+    // Test PROTO mode
+    config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_PROTO;
+    TEST_ASSERT_TRUE(SerialModule::isValidConfig(config));
+
+    // Test NMEA mode
+    config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_NMEA;
+    TEST_ASSERT_TRUE(SerialModule::isValidConfig(config));
+
+    // Test CALTOPO mode
+    config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_CALTOPO;
+    TEST_ASSERT_TRUE(SerialModule::isValidConfig(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_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()
+{
+    initializeTestEnvironment();
+    LOG_WARN("This test requires the ARCH_PORTDUINO variant");
+    UNITY_BEGIN();
+    UNITY_END();
+}
+#endif
+void loop() {}