feat(mqtt): implement downgrade contract for MQTT preferences

Enhance the MQTT preferences handling by establishing a downgrade
contract that ensures compatibility between different firmware versions.
This contract allows nodes to read settings from newer builds while
safeguarding against data loss during downgrades. The implementation
includes updates to the classification logic, ensuring that longer
payloads from newer versions are handled correctly without rejecting
files, thus preserving critical WiFi credentials and broker settings.

MQTT_IMPLEMENTATION.md

@@ -344,11 +344,15 @@ In WebConfig the allowlist is a checkbox per type under each configured slot,
 with **All** / **None** shortcuts. Clearing every box is `none` (nothing
 uploaded).
 
-**Downgrade note:** slots left at the `all` default keep `/mqtt_prefs` in the
-layout older firmware can read. Setting any filter — including `none` — extends
-the file so that a downgrade to pre-filter firmware falls back to defaults and
-loses the stored WiFi credentials along with the broker config. Reset every
-slot to `all` before rolling a node back over the air.
+**Downgrade note:** rolling back to any build from this release onward is safe —
+the older firmware reads the settings it understands and simply ignores the
+packet filters, which revert to `all` if it saves.
+
+Rolling back to a build released *before* this one is the case to watch: that
+firmware rejects the longer settings file outright and falls back to defaults,
+losing the stored WiFi credentials along with the broker config. Slots left at
+the `all` default keep the file in the shorter layout those builds can read, so
+if you may need to roll a node back that far, reset every slot to `all` first.
 
 #### Example: Configure MeshRank on Slot 3
 ```bash

MQTT_INTERNALS.md

@@ -108,27 +108,42 @@ leave the missing tail at its default. The packet-filter addition follows that r
 the prior 2864-byte v1 payload loads with all six filters set to `all`, while the
 full payload is 2876 bytes.
 
-#### Why the written length is not always the full length
+#### The downgrade contract
 
-Reading a longer payload is the easy direction; writing one is a one-way door.
-Pre-filter firmware classifies a 2876-byte v1 payload as `UnsupportedVersion`, so
-after a downgrade the node boots on *defaults* and `_mqtt_prefs_hold` blocks every
-save. `/mqtt_prefs` holds `wifi_ssid`/`wifi_password` as well as the broker config, so
-that node comes up with no network at all: no portal, no OTA, no way back except
-serial or a re-flash forward. The same is true of every previous tail (observer,
-neighbors) — each append moved the cliff.
+**Within a version tag the layout is append-only, and a longer payload is always
+readable.** A file written by a later build starts with this binary's exact baseline,
+so `classify()` reads that prefix and ignores the tail. A downgraded node keeps its
+WiFi credentials, broker slots, and every other setting it understands; the only thing
+it loses is the settings the newer build added.
 
-`MQTTPrefsCodec::payloadLenFor()` narrows the blast radius instead of widening it. It
-returns the *shortest* length that still round-trips the configuration, which for the
-packet-filter tail means: keep writing 2864 while all six masks are the all-types
-default, and only write 2876 once a filter actually holds something. A node that
-upgrades and never touches a filter therefore stays downgrade-readable, and clearing
-the last non-default filter puts it back. Opting in is an explicit operator action,
-not a side effect of installing a build.
+That asymmetry is the whole point. Refusing the file costs the operator the network
+itself — `/mqtt_prefs` holds `wifi_ssid`/`wifi_password` as well as the broker config,
+so a node that falls back to defaults has no WiFi, no portal, and no OTA, recoverable
+only over serial. Reading it costs a feature's settings. Losing later settings is the
+acceptable half of that trade; losing the node is not.
 
-The rule generalises to the next appended field: give it a default that a missing tail
-already implies, and extend `payloadLenFor()` so the longer payload is only written
-when the field is set. That keeps every append reversible for the fleet majority.
+The tail survives until something actually writes. `saveMQTTPrefs()` rewrites at this
+binary's own length, so a rollback that changes no observer setting and is later rolled
+forward keeps the newer fields intact — only an explicit `set` while downgraded drops
+them. The boot log says so when it happens.
+
+**A change that is not a pure append MUST bump `MQTT_PREFS_VERSION`.** The version
+check is what makes the rule above safe: a different tag is still refused outright and
+the file preserved, because the bytes may no longer mean what this binary thinks. Never
+reorder, resize, or repurpose an existing field within a version.
+
+Note the rule is only as old as the build that implements it. Firmware already deployed
+carries the *previous* decoder, which rejects any longer v1 payload — so rolling back
+from this build to one shipped before it still falls back to defaults.
+`MQTTPrefsCodec::payloadLenFor()` covers that gap from the writing side: it returns the
+shortest length that still round-trips the configuration, so a node keeps writing 2864
+bytes until a packet filter actually holds something, and clearing the last non-default
+filter puts it back. That mitigation can be retired once no supported downgrade target
+predates the contract; the contract itself is the durable half.
+
+Shorter payloads keep their existing, stricter treatment: a short length must match a
+boundary that really shipped (`MQTT_PREFS_V1_*_PAYLOAD_SIZE`), because raw prefs have
+no checksum and an arbitrary short size cannot be trusted to mean anything.
 
 ### Settings upgrade / migration
 

src/helpers/CommonCLI.cpp

@@ -793,6 +793,16 @@ void CommonCLI::loadMQTTPrefs(
           MESH_DEBUG_PRINTLN("MQTT: /mqtt_prefs read failed, using defaults (file preserved)");
         } else {
           has_observer_fields = plan.observer_fields_present;
+          // Written by a later build with appended fields. Everything this
+          // binary knows loaded normally; say so, because the next `set` will
+          // rewrite the file at this length and drop the newer settings.
+          if (file_size - sizeof(MQTTPrefsHeader) > plan.payload_len) {
+            MESH_DEBUG_PRINTLN(
+                "MQTT: /mqtt_prefs written by newer firmware (%u > %u bytes); "
+                "config loaded, newer settings ignored and dropped on next save",
+                (unsigned)(file_size - sizeof(MQTTPrefsHeader)),
+                (unsigned)plan.payload_len);
+          }
         }
         if (file) file.close();
       } else if (plan.rewrite_legacy) {

src/helpers/MQTTPrefsCodec.h

@@ -114,10 +114,26 @@ inline DecodePlan classify(const uint8_t* prefix, size_t prefix_read, size_t fil
       if (header.payload_len != payload_available) {
         return corruptPlan();
       }
-      // A same-version append is still unknown to this binary. Holding the
-      // file prevents a downgrade from discarding it on the next CLI save.
+      // A longer same-version payload was written by a later build that
+      // appended fields. Within a version tag the layout is append-only, so
+      // every byte this binary knows is present and correctly positioned — read
+      // the baseline prefix and ignore the tail.
+      //
+      // This is the downgrade contract, and it is deliberately asymmetric:
+      // refusing the file would cost the operator WiFi credentials and every
+      // broker slot (a node with no network and no portal, recoverable only
+      // over serial), whereas reading it costs only the settings the newer
+      // build added. Losing a later feature's settings is the acceptable half.
+      //
+      // The tail survives until something actually writes: saveMQTTPrefs()
+      // rewrites at this binary's own length, so a rollback that changes no
+      // observer setting and is later rolled forward keeps the newer fields
+      // intact. Only an explicit `set` while downgraded drops them.
+      //
+      // A layout change that is NOT a pure append must bump MQTT_PREFS_VERSION;
+      // the version check above is what makes this rule safe.
       if (header.payload_len > kV1BaselinePayloadSize) {
-        return {Source::UnsupportedVersion, false, true, false, 0};
+        return {Source::Current, false, false, true, kV1BaselinePayloadSize};
       }
       if (header.payload_len == kV1BaselinePayloadSize) {
         return {Source::Current, false, false, true, kV1BaselinePayloadSize};

src/helpers/MQTTPrefsStorage.h

@@ -135,7 +135,14 @@ static const uint32_t MQTT_NEIGHBORS_MAX_INTERVAL_MS = MQTT_NEIGHBORS_MAX_INTERV
 static const uint32_t MQTT_NEIGHBORS_DEFAULT_INTERVAL_MS = MQTT_NEIGHBORS_DEFAULT_INTERVAL_HOURS * 3600000UL;
 
 // Version-1 has four payload layouts this firmware can decode. Never infer a
-// compatible payload from an arbitrary shorter size: raw prefs have no checksum.
+// compatible payload from an arbitrary SHORTER size: raw prefs have no
+// checksum, so a short length has to match a boundary that was really shipped.
+//
+// A LONGER v1 payload is different and is always readable: within a version tag
+// the layout is append-only, so a later build's file still starts with this
+// binary's exact baseline. classify() reads that prefix and ignores the tail
+// rather than rejecting the file — see the downgrade contract there. Any change
+// that is not a pure append MUST bump MQTT_PREFS_VERSION instead.
 //   - PRE_OBSERVER  (2736): stops before the observer tail (snmp_*/alert_*).
 //   - PRE_NEIGHBORS (2860): full observer tail, no neighbors fields yet.
 //   - PRE_FILTER    (2864): neighbors tail, no per-slot packet filters.

test/test_mqtt_prefs_codec/test_mqtt_prefs_codec.cpp

@@ -499,18 +499,83 @@ TEST(MQTTPrefsCodec, UnsupportedHeaderlessSizesArePreserved) {
   }
 }
 
-TEST(MQTTPrefsCodec, NewerAndSameVersionExtendedPayloadsAreHeld) {
+// A different version tag means the layout may have changed shape, so it is
+// still refused outright. That refusal is what makes reading longer same-version
+// payloads safe, so the two belong in one test.
+TEST(MQTTPrefsCodec, ADifferentVersionTagIsStillRefusedAndHeld) {
   std::vector<uint8_t> newer(sizeof(MQTTPrefsHeader), 0);
   writeHeader(&newer, MQTT_PREFS_VERSION + 1, 0);
-  Codec::DecodePlan plan = classify(newer);
+  const Codec::DecodePlan plan = classify(newer);
   EXPECT_EQ(Codec::Source::UnsupportedVersion, plan.source);
   EXPECT_TRUE(plan.preserve_file);
+}
 
-  std::vector<uint8_t> extended(sizeof(MQTTPrefsHeader) + Codec::kV1BaselinePayloadSize + 1, 0);
-  writeHeader(&extended, MQTT_PREFS_VERSION,
-              static_cast<uint16_t>(Codec::kV1BaselinePayloadSize + 1));
-  plan = classify(extended);
-  EXPECT_EQ(Codec::Source::UnsupportedVersion, plan.source);
+// The downgrade contract: a v1 payload longer than this build's baseline was
+// appended to by a later build, so the baseline prefix is present verbatim.
+// Read it and drop the tail — never refuse the file, which would cost the
+// operator WiFi and every broker slot to save settings they don't understand.
+TEST(MQTTPrefsCodec, LongerSameVersionPayloadLoadsTheBaselineAndIgnoresTheTail) {
+  MQTTPrefs source = defaults();
+  strncpy(source.mqtt_origin, "future-node", sizeof(source.mqtt_origin) - 1);
+  strncpy(source.wifi_ssid, "field-ssid", sizeof(source.wifi_ssid) - 1);
+  strncpy(source.wifi_password, "field-secret", sizeof(source.wifi_password) - 1);
+  strncpy(source.mqtt_iata, "SEA", sizeof(source.mqtt_iata) - 1);
+  strncpy(source.mqtt_slot_preset[0], "meshrank", sizeof(source.mqtt_slot_preset[0]) - 1);
+  source.mqtt_neighbors_enabled = 1;
+  for (int i = 0; i < MQTT_PREFS_SLOT_COUNT; ++i) {
+    source.mqtt_slot_packet_filter[i] = static_cast<uint16_t>(1u << i);
+  }
+
+  // Baseline image plus a 40-byte tail of fields this build has never heard of.
+  const size_t kTail = 40;
+  const size_t payload_len = Codec::kV1BaselinePayloadSize + kTail;
+  std::vector<uint8_t> bytes(sizeof(MQTTPrefsHeader) + payload_len, 0xA5);
+  writeHeader(&bytes, MQTT_PREFS_VERSION, static_cast<uint16_t>(payload_len));
+  memcpy(bytes.data() + sizeof(MQTTPrefsHeader), &source, sizeof(source));
+
+  const Codec::DecodePlan plan = classify(bytes);
+  ASSERT_EQ(Codec::Source::Current, plan.source);
+  EXPECT_FALSE(plan.preserve_file) << "refusing the file would strand the node";
+  EXPECT_TRUE(plan.observer_fields_present);
+  EXPECT_FALSE(plan.rewrite_legacy);
+  ASSERT_EQ(Codec::kV1BaselinePayloadSize, plan.payload_len);
+
+  // Reading plan.payload_len bytes recovers this build's whole struct exactly,
+  // and cannot run past it into the unknown tail.
+  MQTTPrefs loaded = defaults();
+  memcpy(&loaded, bytes.data() + sizeof(MQTTPrefsHeader), plan.payload_len);
+  EXPECT_EQ(0, memcmp(&source, &loaded, sizeof(source)));
+  EXPECT_STREQ("future-node", loaded.mqtt_origin);
+  EXPECT_STREQ("field-ssid", loaded.wifi_ssid);
+  EXPECT_STREQ("field-secret", loaded.wifi_password);
+  EXPECT_STREQ("meshrank", loaded.mqtt_slot_preset[0]);
+  EXPECT_EQ(1u, loaded.mqtt_neighbors_enabled);
+  EXPECT_EQ(1u, loaded.mqtt_slot_packet_filter[0]);
+}
+
+// The rule has to hold for any appended size, including a single byte and a
+// tail far larger than the baseline.
+TEST(MQTTPrefsCodec, EveryLongerSameVersionLengthReadsTheBaseline) {
+  for (const size_t tail : {size_t(1), size_t(2), size_t(12), size_t(64),
+                            size_t(512), size_t(4096)}) {
+    const size_t payload_len = Codec::kV1BaselinePayloadSize + tail;
+    std::vector<uint8_t> bytes(sizeof(MQTTPrefsHeader) + payload_len, 0);
+    writeHeader(&bytes, MQTT_PREFS_VERSION, static_cast<uint16_t>(payload_len));
+    const Codec::DecodePlan plan = classify(bytes);
+    EXPECT_EQ(Codec::Source::Current, plan.source) << tail;
+    EXPECT_FALSE(plan.preserve_file) << tail;
+    EXPECT_EQ(Codec::kV1BaselinePayloadSize, plan.payload_len) << tail;
+  }
+}
+
+// A length that merely *declares* a longer payload without the bytes to back it
+// is still corrupt — the header/file-size agreement check must run first.
+TEST(MQTTPrefsCodec, LongerDeclaredLengthWithoutTheBytesIsStillCorrupt) {
+  std::vector<uint8_t> bytes(sizeof(MQTTPrefsHeader) + Codec::kV1BaselinePayloadSize, 0);
+  writeHeader(&bytes, MQTT_PREFS_VERSION,
+              static_cast<uint16_t>(Codec::kV1BaselinePayloadSize + 16));
+  const Codec::DecodePlan plan = classify(bytes);
+  EXPECT_EQ(Codec::Source::Corrupt, plan.source);
   EXPECT_TRUE(plan.preserve_file);
 }