diff --git a/MQTT_IMPLEMENTATION.md b/MQTT_IMPLEMENTATION.md index ded092c6..e772008d 100644 --- a/MQTT_IMPLEMENTATION.md +++ b/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 diff --git a/MQTT_INTERNALS.md b/MQTT_INTERNALS.md index 1de6e00c..35809219 100644 --- a/MQTT_INTERNALS.md +++ b/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 diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index c2f23fe1..7e786f23 100644 --- a/src/helpers/CommonCLI.cpp +++ b/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) { diff --git a/src/helpers/MQTTPrefsCodec.h b/src/helpers/MQTTPrefsCodec.h index 57e19890..7bbfe30a 100644 --- a/src/helpers/MQTTPrefsCodec.h +++ b/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}; diff --git a/src/helpers/MQTTPrefsStorage.h b/src/helpers/MQTTPrefsStorage.h index 88119077..e3c6a098 100644 --- a/src/helpers/MQTTPrefsStorage.h +++ b/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. diff --git a/test/test_mqtt_prefs_codec/test_mqtt_prefs_codec.cpp b/test/test_mqtt_prefs_codec/test_mqtt_prefs_codec.cpp index 103f6825..7bf49746 100644 --- a/test/test_mqtt_prefs_codec/test_mqtt_prefs_codec.cpp +++ b/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 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 extended(sizeof(MQTTPrefsHeader) + Codec::kV1BaselinePayloadSize + 1, 0); - writeHeader(&extended, MQTT_PREFS_VERSION, - static_cast(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(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 bytes(sizeof(MQTTPrefsHeader) + payload_len, 0xA5); + writeHeader(&bytes, MQTT_PREFS_VERSION, static_cast(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 bytes(sizeof(MQTTPrefsHeader) + payload_len, 0); + writeHeader(&bytes, MQTT_PREFS_VERSION, static_cast(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 bytes(sizeof(MQTTPrefsHeader) + Codec::kV1BaselinePayloadSize, 0); + writeHeader(&bytes, MQTT_PREFS_VERSION, + static_cast(Codec::kV1BaselinePayloadSize + 16)); + const Codec::DecodePlan plan = classify(bytes); + EXPECT_EQ(Codec::Source::Corrupt, plan.source); EXPECT_TRUE(plan.preserve_file); }