Reject invalid configuration for the default MQTT server (#6066)

* Sanity check configuration for the default MQTT server

* Skip for MESHTASTIC_EXCLUDE_MQTT

---------

Co-authored-by: Ben Meadors <benmmeadors@gmail.com>

src/modules/AdminModule.cpp

@@ -162,7 +162,9 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta
 
     case meshtastic_AdminMessage_set_module_config_tag:
         LOG_INFO("Client set module config");
-        handleSetModuleConfig(r->set_module_config);
+        if (!handleSetModuleConfig(r->set_module_config)) {
+            myReply = allocErrorResponse(meshtastic_Routing_Error_BAD_REQUEST, &mp);
+        }
         break;
 
     case meshtastic_AdminMessage_set_channel_tag:
@@ -648,15 +650,23 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c)
     saveChanges(changes, requiresReboot);
 }
 
-void AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c)
+bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c)
 {
     if (!hasOpenEditTransaction)
         disableBluetooth();
     switch (c.which_payload_variant) {
     case meshtastic_ModuleConfig_mqtt_tag:
+#if MESHTASTIC_EXCLUDE_MQTT
+        LOG_WARN("Set module config: MESHTASTIC_EXCLUDE_MQTT is defined. Not setting MQTT config");
+        return false;
+#else
         LOG_INFO("Set module config: MQTT");
+        if (!MQTT::isValidConfig(c.payload_variant.mqtt)) {
+            return false;
+        }
         moduleConfig.has_mqtt = true;
         moduleConfig.mqtt = c.payload_variant.mqtt;
+#endif
         break;
     case meshtastic_ModuleConfig_serial_tag:
         LOG_INFO("Set module config: Serial");
@@ -724,6 +734,7 @@ void AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c)
         break;
     }
     saveChanges(SEGMENT_MODULECONFIG);
+    return true;
 }
 
 void AdminModule::handleSetChannel(const meshtastic_Channel &cc)
@@ -1160,4 +1171,4 @@ void disableBluetooth()
         nrf52Bluetooth->shutdown();
 #endif
 #endif
-}
+}

src/modules/AdminModule.h

@@ -50,7 +50,7 @@ class AdminModule : public ProtobufModule<meshtastic_AdminMessage>, public Obser
     void handleSetOwner(const meshtastic_User &o);
     void handleSetChannel(const meshtastic_Channel &cc);
     void handleSetConfig(const meshtastic_Config &c);
-    void handleSetModuleConfig(const meshtastic_ModuleConfig &c);
+    bool handleSetModuleConfig(const meshtastic_ModuleConfig &c);
     void handleSetChannel();
     void handleSetHamMode(const meshtastic_HamParameters &req);
     void handleStoreDeviceUIConfig(const meshtastic_DeviceUIConfig &uicfg);

src/mqtt/MQTT.cpp

@@ -41,6 +41,7 @@ MQTT *mqtt;
 namespace
 {
 constexpr int reconnectMax = 5;
+constexpr uint16_t mqttPort = 1883;
 
 // FIXME - this size calculation is super sloppy, but it will go away once we dynamically alloc meshpackets
 static uint8_t bytes[meshtastic_MqttClientProxyMessage_size + 30]; // 12 for channel name and 16 for nodeid
@@ -245,6 +246,11 @@ std::pair<String, uint16_t> parseHostAndPort(String server, uint16_t port = 0)
     }
     return std::make_pair(std::move(server), port);
 }
+
+bool isDefaultServer(const String &host)
+{
+    return host.length() == 0 || host == default_mqtt_address;
+}
 } // namespace
 
 void MQTT::mqttCallback(char *topic, byte *payload, unsigned int length)
@@ -324,7 +330,7 @@ MQTT::MQTT() : concurrency::OSThread("mqtt"), mqttQueue(MAX_MQTT_QUEUE)
         }
 
         String host = parseHostAndPort(moduleConfig.mqtt.address).first;
-        isConfiguredForDefaultServer = host.length() == 0 || host == default_mqtt_address;
+        isConfiguredForDefaultServer = isDefaultServer(host);
         IPAddress ip;
         isMqttServerAddressPrivate = ip.fromString(host.c_str()) && isPrivateIpAddress(ip);
 
@@ -408,7 +414,7 @@ void MQTT::reconnect()
         }
 #if HAS_NETWORKING
         // Defaults
-        int serverPort = 1883;
+        int serverPort = mqttPort;
         const char *serverAddr = default_mqtt_address;
         const char *mqttUsername = default_mqtt_username;
         const char *mqttPassword = default_mqtt_password;
@@ -561,6 +567,23 @@ int32_t MQTT::runOnce()
     return 30000;
 }
 
+bool MQTT::isValidConfig(const meshtastic_ModuleConfig_MQTTConfig &config)
+{
+    String host;
+    uint16_t port;
+    std::tie(host, port) = parseHostAndPort(config.address, mqttPort);
+    const bool defaultServer = isDefaultServer(host);
+    if (defaultServer && config.tls_enabled) {
+        LOG_ERROR("Invalid MQTT config: TLS was enabled, but the default server does not support TLS");
+        return false;
+    }
+    if (defaultServer && port != mqttPort) {
+        LOG_ERROR("Invalid MQTT config: Unsupported port '%d' for the default MQTT server", port);
+        return false;
+    }
+    return true;
+}
+
 void MQTT::publishNodeInfo()
 {
     // TODO: NodeInfo broadcast over MQTT only (NODENUM_BROADCAST_NO_LORA)

src/mqtt/MQTT.h

@@ -61,6 +61,8 @@ class MQTT : private concurrency::OSThread
 
     bool isUsingDefaultServer() { return isConfiguredForDefaultServer; }
 
+    static bool isValidConfig(const meshtastic_ModuleConfig_MQTTConfig &config);
+
   protected:
     struct QueueEntry {
         std::string topic;

test/test_mqtt/MQTT.cpp

@@ -800,6 +800,38 @@ void test_customMqttRoot(void)
         [] { return pubsub->subscriptions_.count("custom/2/e/test/+") && pubsub->subscriptions_.count("custom/2/e/PKI/+"); }));
 }
 
+// Empty configuration is valid.
+void test_configurationEmptyIsValid(void)
+{
+    meshtastic_ModuleConfig_MQTTConfig config;
+
+    TEST_ASSERT_TRUE(MQTT::isValidConfig(config));
+}
+
+// Configuration with the default server is valid.
+void test_configWithDefaultServer(void)
+{
+    meshtastic_ModuleConfig_MQTTConfig config = {.address = default_mqtt_address};
+
+    TEST_ASSERT_TRUE(MQTT::isValidConfig(config));
+}
+
+// Configuration with the default server and port 8888 is invalid.
+void test_configWithDefaultServerAndInvalidPort(void)
+{
+    meshtastic_ModuleConfig_MQTTConfig config = {.address = default_mqtt_address ":8888"};
+
+    TEST_ASSERT_FALSE(MQTT::isValidConfig(config));
+}
+
+// Configuration with the default server and tls_enabled = true is invalid.
+void test_configWithDefaultServerAndInvalidTLSEnabled(void)
+{
+    meshtastic_ModuleConfig_MQTTConfig config = {.tls_enabled = true};
+
+    TEST_ASSERT_FALSE(MQTT::isValidConfig(config));
+}
+
 void setup()
 {
     initializeTestEnvironment();
@@ -843,6 +875,10 @@ void setup()
     RUN_TEST(test_enabled);
     RUN_TEST(test_disabled);
     RUN_TEST(test_customMqttRoot);
+    RUN_TEST(test_configurationEmptyIsValid);
+    RUN_TEST(test_configWithDefaultServer);
+    RUN_TEST(test_configWithDefaultServerAndInvalidPort);
+    RUN_TEST(test_configWithDefaultServerAndInvalidTLSEnabled);
     exit(UNITY_END());
 }
 #else