Add helpful checks of channel names and PSK (#10792)

* added warnings from firmware for simple channel setting mistakes

* more and better checks

* one ping only

* Drop literal 'AQ==' from blank-PSK channel warnings

The base64 encoding of the default channel key isn't something a user
should type in by hand; state the condition instead of a raw key value.

---------

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

src/modules/AdminModule.cpp

@@ -1,5 +1,6 @@
 #include "AdminModule.h"
 #include "Channels.h"
+#include "DisplayFormatters.h"
 #include "MeshService.h"
 #include "NodeDB.h"
 #include "PositionPrecision.h"
@@ -461,6 +462,7 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta
         LOG_INFO("Commit transaction for edited settings");
         hasOpenEditTransaction = false;
         saveChanges(SEGMENT_CONFIG | SEGMENT_MODULECONFIG | SEGMENT_DEVICESTATE | SEGMENT_CHANNELS | SEGMENT_NODEDATABASE);
+        flushChannelWarnings(); // one coalesced message for everything edited in this transaction
         break;
     }
     case meshtastic_AdminMessage_get_device_connection_status_request_tag: {
@@ -755,7 +757,7 @@ void AdminModule::handleSetOwner(const meshtastic_User &o)
         changed = 1;
         owner.is_licensed = o.is_licensed;
         if (channels.ensureLicensedOperation()) {
-            sendWarning(licensedModeMessage);
+            warnLicensedMode();
         }
     }
     if (owner.has_is_unmessagable != o.has_is_unmessagable ||
@@ -777,6 +779,8 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
     auto existingRole = config.device.role;
     bool isRegionUnset = (config.lora.region == meshtastic_Config_LoRaConfig_RegionCode_UNSET);
     bool requiresReboot = true;
+    bool loraPresetWarnPending = false;
+    meshtastic_Config_LoRaConfig pendingOldLora = {}, pendingNewLora = {};
 
     switch (c.which_payload_variant) {
     case meshtastic_Config_device_tag: {
@@ -1041,6 +1045,11 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
 #endif
 
         config.lora = validatedLora; // Finally, return the validated config back to the main config
+        if (validatedLora.modem_preset != oldLoraConfig.modem_preset) {
+            pendingOldLora = oldLoraConfig;
+            pendingNewLora = validatedLora;
+            loraPresetWarnPending = true;
+        }
 
         break;
     }
@@ -1093,6 +1102,11 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
     } // end of switch case which_payload_variant
 
     saveChanges(changes, requiresReboot);
+    if (loraPresetWarnPending)
+        warnOnLoraPresetChange(pendingOldLora, pendingNewLora);
+    // Inside an edit transaction the queued warnings are flushed once at commit; otherwise emit now.
+    if (!hasOpenEditTransaction)
+        flushChannelWarnings();
 } // end of handleSetConfig
 
 bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c)
@@ -1298,7 +1312,7 @@ void AdminModule::handleSetChannel(const meshtastic_Channel &cc)
 {
     channels.setChannel(cc);
     if (channels.ensureLicensedOperation()) {
-        sendWarning(licensedModeMessage);
+        warnLicensedMode();
     }
     // Refresh derived state (primaryIndex in particular) BEFORE the precision clamp below. usesPublicKey()
     // resolves a secondary channel's key against the primary, so it must see the post-update primaryIndex;
@@ -1321,6 +1335,10 @@ void AdminModule::handleSetChannel(const meshtastic_Channel &cc)
     if (clamped)
         sendWarning(publicChannelPrecisionMessage);
     saveChanges(SEGMENT_CHANNELS, false);
+    warnOnChannelSet(channels.getByIndex(cc.index)); // passes the saved channel
+    // Inside an edit transaction the queued warnings are flushed once at commit; otherwise emit now.
+    if (!hasOpenEditTransaction)
+        flushChannelWarnings();
 }
 
 /**
@@ -1736,7 +1754,7 @@ void AdminModule::handleSetHamMode(const meshtastic_HamParameters &p)
     // Remove PSK of primary channel for plaintext amateur usage
 
     if (channels.ensureLicensedOperation()) {
-        sendWarning(licensedModeMessage);
+        warnLicensedMode();
     }
     channels.onConfigChanged();
 
@@ -1874,14 +1892,259 @@ void AdminModule::sendWarningAndLog(const char *format, ...)
     vsnprintf(buf, sizeof(buf), format, args);
     va_end(args);
 
-    LOG_WARN(buf);
-    // 2. Call sendWarning
-    // SECURITY NOTE: We pass "%s", buf instead of just 'buf'.
-    // If 'buf' contained a % symbol (e.g. "Battery 50%"), passing it directly
-    // would crash sendWarning. "%s" treats it purely as text.
+    // SECURITY NOTE: Both LOG_WARN and sendWarning are printf-style, so we pass
+    // "%s", buf rather than 'buf' directly. If 'buf' contained a % symbol (e.g. a
+    // user-supplied channel name like "50%"), passing it as the format string would
+    // read bogus varargs and could crash. "%s" treats it purely as text.
+    LOG_WARN("%s", buf);
     sendWarning("%s", buf);
 }
 
+// Strip spaces and fold to lowercase for loose preset-name comparison.
+static void normalizePresetName(const char *src, char *dst, size_t dstLen)
+{
+    size_t j = 0;
+    for (size_t i = 0; src[i] && j + 1 < dstLen; i++) {
+        if (src[i] != ' ')
+            dst[j++] = (char)tolower((unsigned char)src[i]);
+    }
+    dst[j] = '\0';
+}
+
+// Record one channel-configuration warning. The first message is kept verbatim in case it
+// turns out to be the only one; nameIssue/pskIssue and the channel bitmask feed the catch-all
+// wording if more than one channel ends up flagged. flushChannelWarnings() emits the result.
+void AdminModule::queueChannelWarning(uint8_t channelIndex, bool nameIssue, bool pskIssue, const char *format, ...)
+{
+    if (pendingWarningCount == 0) {
+        va_list args;
+        va_start(args, format);
+        vsnprintf(pendingWarningText, sizeof(pendingWarningText), format, args);
+        va_end(args);
+    }
+    if (channelIndex < MAX_NUM_CHANNELS)
+        pendingWarningChannels |= (1u << channelIndex);
+    pendingWarningNameIssue |= nameIssue;
+    pendingWarningPskIssue |= pskIssue;
+    pendingWarningCount++;
+}
+
+// Queue the fixed "licensed mode activated" notice, deferring it to commit during an edit
+// transaction so repeated triggers collapse to a single message.
+void AdminModule::warnLicensedMode()
+{
+    if (hasOpenEditTransaction)
+        pendingLicenseWarning = true;
+    else
+        sendWarning(licensedModeMessage);
+}
+
+// Emit the coalesced channel warning(s): nothing if none queued, the lone message verbatim if
+// exactly one, otherwise a single catch-all naming every flagged channel. The licensed-mode
+// notice, if queued, is emitted once alongside. Always resets state.
+void AdminModule::flushChannelWarnings()
+{
+    if (pendingLicenseWarning)
+        sendWarning(licensedModeMessage);
+
+    if (pendingWarningCount == 1) {
+        sendWarningAndLog("%s", pendingWarningText);
+    } else if (pendingWarningCount > 1) {
+        char list[48] = {};
+        for (uint8_t i = 0; i < MAX_NUM_CHANNELS; i++) {
+            if (!(pendingWarningChannels & (1u << i)))
+                continue;
+            char num[8];
+            snprintf(num, sizeof(num), "%s%u", *list ? ", " : "", i);
+            strncat(list, num, sizeof(list) - strlen(list) - 1);
+        }
+        if (pendingWarningNameIssue && pendingWarningPskIssue)
+            sendWarningAndLog("There may be name and PSK issues on channels %s", list); // max 60 bytes
+        else if (pendingWarningNameIssue)
+            sendWarningAndLog("There may be name issues on channels %s", list); // max 52 bytes
+        else
+            sendWarningAndLog("There may be PSK issues on channels %s", list); // max 51 bytes
+    }
+    pendingWarningText[0] = '\0';
+    pendingWarningChannels = 0;
+    pendingWarningCount = 0;
+    pendingWarningNameIssue = false;
+    pendingWarningPskIssue = false;
+    pendingLicenseWarning = false;
+}
+
+/**
+ * @brief Emit client warnings for common misconfigurations on a newly committed channel.
+ *
+ * Called from handleSetChannel() after the channel has been saved. The following checks
+ * are performed:
+ *
+ * - Blank PSK (size == 0) on a non-licensed device: the channel has no encryption.
+ * - Blank name with a non-default key (not the default key, 0x01): the name will auto-resolve to
+ *   the current modem preset name, but the key mismatch means other preset nodes cannot
+ *   decode traffic on this channel.
+ * - Named channel whose name is a case/space variant of the current modem preset: the
+ *   explicit name prevents auto-resolution; client should clear it.
+ * - Same variant match but PSK is not the default key: looks like the preset channel but
+ *   is incompatible with nodes using the preset's default key.
+ *
+ * @param cc  The channel that was written.
+ */
+void AdminModule::warnOnChannelSet(const meshtastic_Channel &cc)
+{
+    if (cc.role == meshtastic_Channel_Role_DISABLED || !cc.has_settings) // don't check unused channels
+        return;
+
+    bool blankPsk = (cc.settings.psk.size == 0 && !owner.is_licensed);
+
+    if (!config.lora.use_preset) { // custom or unset preset can mistype things too
+        const char *mistypePreset = nullptr;
+        if (*cc.settings.name) {
+            char normChan[32];
+            normalizePresetName(cc.settings.name, normChan, sizeof(normChan));
+            for (auto preset = _meshtastic_Config_LoRaConfig_ModemPreset_MIN;
+                 preset <= _meshtastic_Config_LoRaConfig_ModemPreset_MAX;
+                 preset = (meshtastic_Config_LoRaConfig_ModemPreset)(preset + 1)) {
+                const char *name = DisplayFormatters::getModemPresetDisplayName(preset, false, true);
+                if (strcmp(name, "Invalid") == 0)
+                    continue; // skip preset slots without a real display name
+                if (strcmp(cc.settings.name, name) == 0)
+                    break; // exact match - not a mistype, no warning
+                char normPreset[32];
+                normalizePresetName(name, normPreset, sizeof(normPreset));
+                if (strcmp(normChan, normPreset) == 0) {
+                    mistypePreset = name;
+                    break;
+                }
+            }
+        }
+        // At most one warning: collapse blank-PSK + mistype into a single catch-all.
+        if (blankPsk && mistypePreset)
+            queueChannelWarning(cc.index, true, true, "There may be name and PSK issues on channel %d", cc.index);
+        else if (mistypePreset)
+            queueChannelWarning(cc.index, true, false,
+                                "Channel %d name '%s' looks like a mistype of '%s' - "
+                                "make sure to type it exactly!", // max 90 bytes
+                                cc.index, cc.settings.name, mistypePreset);
+        else if (blankPsk)
+            queueChannelWarning(cc.index, false, true,
+                                "Channel %d '%s' has a blank PSK (no encryption) instead of default key", // max 100 bytes
+                                cc.index, cc.settings.name);
+        return;
+    }
+
+    const char *presetName = DisplayFormatters::getModemPresetDisplayName(config.lora.modem_preset, false, true);
+    bool isDefaultKey = (cc.settings.psk.size == 1 && cc.settings.psk.bytes[0] == 0x01);
+    char normPreset[32], normChan[32]; // max size is 11 plus nul, but allow for future expansion
+    normalizePresetName(presetName, normPreset, sizeof(normPreset));
+    normalizePresetName(cc.settings.name, normChan, sizeof(normChan));
+
+    if (!*cc.settings.name) {
+        // Blank name resolves to the preset name - A and B are mutually exclusive (psk.size can't be both 0 and >0)
+        if (blankPsk)
+            queueChannelWarning(cc.index, false, true,
+                                "Channel %d '%s' has a blank PSK (no encryption) instead of default key", // max 100 bytes
+                                cc.index, cc.settings.name);
+        else if (!isDefaultKey && cc.settings.psk.size > 0)
+            queueChannelWarning(cc.index, false, true,
+                                "Channel %d will resolve to preset '%s' but uses a non-default key - "
+                                "default-key nodes can't decode it.", // max 102 bytes
+                                cc.index, presetName);
+        return;
+    }
+
+    if (strcmp(normChan, normPreset) != 0) { // name unrelated to preset
+        if (blankPsk)
+            queueChannelWarning(cc.index, false, true,
+                                "Channel %d '%s' has a blank PSK (no encryption) instead of default key", // max 100 bytes
+                                cc.index, cc.settings.name);
+        return;
+    }
+
+    bool variantName = (strcmp(cc.settings.name, presetName) != 0);
+    bool keyMismatch = (!isDefaultKey && cc.settings.psk.size > 0);
+    int issues = (int)blankPsk + (int)variantName + (int)keyMismatch;
+
+    if (issues > 1) {
+        bool hasNameIssue = variantName;
+        bool hasPskIssue = blankPsk || keyMismatch;
+        if (hasNameIssue && hasPskIssue)
+            queueChannelWarning(cc.index, true, true, "There may be name and PSK issues on channel %d", cc.index);
+        else if (hasNameIssue)
+            queueChannelWarning(cc.index, true, false, "There may be name issues on channel %d", cc.index);
+        else
+            queueChannelWarning(cc.index, false, true, "There may be PSK issues on channel %d", cc.index);
+        return;
+    }
+
+    if (blankPsk)
+        queueChannelWarning(cc.index, false, true,
+                            "Channel %d '%s' has a blank PSK (no encryption) instead of default key", // max 100 bytes
+                            cc.index, cc.settings.name);
+    if (variantName)
+        queueChannelWarning(cc.index, true, false,
+                            "Channel %d name '%s' looks like a mistype of '%s' - "
+                            "clear the name to use the preset name automatically.", // max 113 bytes
+                            cc.index, cc.settings.name, presetName);
+    if (keyMismatch)
+        queueChannelWarning(cc.index, false, true,
+                            "Channel %d '%s' matches preset '%s' but uses a non-default key - "
+                            "default-key nodes can't decode it.", // max 108 bytes
+                            cc.index, cc.settings.name, presetName);
+} // warnOnChannelSet
+
+/**
+ * @brief Scan all channels for preset-name conflicts after a modem preset change is committed.
+ *
+ * Called from handleSetConfig() after the LoRa config has been saved, and only when
+ * modem_preset actually changed (rejected configs are never passed here). For every
+ * named, non-disabled channel two checks are performed:
+ *
+ * - Name matches the *old* preset (case-insensitive, spaces stripped): the channel
+ *   was likely tracking the previous preset; the user should rename it if it should
+ *   follow the new one.
+ * - Name matches the *new* preset: the channel name collides with the auto-generated
+ *   preset name but won't resolve automatically because the name is set explicitly.
+ *
+ * No-ops if the new config does not use a preset.
+ *
+ * @param oldLora  LoRa config before the update.
+ * @param newLora  LoRa config after the update.
+ */
+void AdminModule::warnOnLoraPresetChange(const meshtastic_Config_LoRaConfig &oldLora, const meshtastic_Config_LoRaConfig &newLora)
+{
+    if (!newLora.use_preset || newLora.modem_preset == oldLora.modem_preset)
+        return;
+
+    char normOld[32] = {}, normNew[32];
+    if (oldLora.use_preset) {
+        const char *oldName = DisplayFormatters::getModemPresetDisplayName(oldLora.modem_preset, false, true);
+        normalizePresetName(oldName, normOld, sizeof(normOld));
+    }
+    const char *newName = DisplayFormatters::getModemPresetDisplayName(newLora.modem_preset, false, true);
+    normalizePresetName(newName, normNew, sizeof(normNew));
+
+    // Queue one (name) warning per affected channel; flushChannelWarnings() collapses them
+    // into a single message - either the lone warning verbatim or a catch-all listing indices.
+    for (int i = 0; i < channels.getNumChannels(); i++) {
+        const meshtastic_Channel &ch = channels.getByIndex(i);
+        if (ch.role == meshtastic_Channel_Role_DISABLED || !ch.has_settings || !*ch.settings.name)
+            continue;
+        char normChan[32];
+        normalizePresetName(ch.settings.name, normChan, sizeof(normChan));
+        if (*normOld && strcmp(normChan, normOld) == 0)
+            queueChannelWarning(i, true, false,
+                                "Channel %d name '%s' matches the old preset. "
+                                "Rename it manually if it should track the new preset.", // max 98 bytes
+                                i, ch.settings.name);
+        else if (strcmp(normChan, normNew) == 0)
+            queueChannelWarning(i, true, false,
+                                "Channel %d '%s' looks like preset '%s' but won't auto-resolve - "
+                                "clear the name to fix it.", // max 98 bytes
+                                i, ch.settings.name, newName);
+    }
+} // warnOnLoraPresetChange
+
 void disableBluetooth()
 {
 #if HAS_BLUETOOTH