fix(NodeDB): require a full 32-byte key when demoting to the warm tier (#11431)

* fix(NodeDB): require a full 32-byte key when demoting to the warm tier

meshtastic_User.public_key is a wire `bytes` field with max_size 32, so any
size in 0..32 decodes off the air, and nothing validates it on ingress:
NodeInfoModule hands the decoded User straight to NodeDB::updateUser, whose
PKI gates are all `== 32` and so fall through for a partial key, and
TypeConversions::CopyUserToNodeInfoLite then stores it with the short size.

demoteOldestHotNodesToWarm() admitted that partial key into the warm tier on
a `size > 0` gate. WarmNodeEntry has no length field - it distinguishes "has
a key" from "no key" purely by all-zero - so N real bytes plus 32-N zeros
become indistinguishable from a genuine key. copyPublicKeyAuthoritative()
then hands that fabricated key back with size = 32 and reports it
AUTHORITATIVE, and re-admission writes size = 32 into the hot store. From
then on updateUser's key pin permanently rejects the node's real NodeInfo,
and DMs to it are encrypted to a key nobody holds.

Require a full 32-byte key, so a partial one is absorbed as "no key"
(nullptr) rather than as a truncated one. WarmNodeStore::place() already
treats a null key as keyless and clears the slot's stale key when
repurposing it. This aligns the site with its two siblings, which both
already gate on `size == 32` (the purge path in cleanupMeshDB and the
runtime eviction in getOrCreateMeshNode).

The ingress gap - updateUser accepting a 1..31-byte key at all - is a
separate, larger change and is left for its own review.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(NodeDB): shorten warm-demotion comment to two lines

Repo guideline (AGENTS.md): keep code comments to one or two lines. Retains the
non-obvious invariant - warm entries have no key length field - and drops the
restated detail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test(NodeDB): cover short-key demotion into the warm tier

A warm record stores 32 raw key bytes with no length field, so a partial
hot-store key is indistinguishable from a real one once demoted. The
public_key.size == 32 gate in demoteOldestHotNodesToWarm() is what keeps a
truncated key from being laundered into a full-looking warm key, but nothing
exercised it.

test_migration_dropsShortKeyOnDemotion overflows the hot store with one node
carrying a 31-byte key and asserts it lands as a keyless placeholder while a
genuine 32-byte key still survives. push() grows a keySize parameter to seed
the partial key, and clearWarm() gives the test an empty warm tier, which it
needs because the warm store outlives setUp() and a prior run's warm.dat.

Verified to discriminate: with the size gate reverted to size > 0 the new
test fails on "a 31-byte key must not be demoted as if it were a full key",
and passes again once restored.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test(NodeDB): assert the keyless placeholder carries last_heard

The test only proved a warm metadata row survived the demotion, not that the
placeholder does the job the nullptr is there for, which is preserving
last_heard when the key is dropped.

Asserting the value needed the seeds fixing first. Warm entries pack role,
protected category and the xeddsa flag into the low 7 bits of last_heard
(WARM_TIME_MASK is 0xFFFFFF80), so warm time has 128 second granularity and the
old seeds of 1, 2, 3 all quantised to 0. They are now multiples of 128, which
keeps the demotion ordering identical and makes the values survive the round
trip. Real last_heard is epoch seconds, so this is closer to production than
the old counter was.

Reads the entry through WarmNodeStore::take() rather than getOrCreateMeshNode(),
which does not restore last_heard from the warm tier and would have been
asserting a path that does not exist.

Reported by CodeRabbit on #11431.

---------

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

src/mesh/NodeDB.cpp

@@ -2148,9 +2148,9 @@ void NodeDB::demoteOldestHotNodesToWarm()
         const meshtastic_NodeInfoLite &n = (*meshNodes)[i];
         if (n.num == 0)
             continue;
-        // Keep the public key if we have one (40 B warm record); keyless nodes
-        // still get a placeholder so re-admission restores last_heard.
-        warmStore.absorb(n.num, n.last_heard, n.public_key.size > 0 ? n.public_key.bytes : nullptr, n.role,
+        // Warm entries carry no key length, so a partial key would be indistinguishable
+        // from a full one. nullptr keeps the keyless placeholder that restores last_heard.
+        warmStore.absorb(n.num, n.last_heard, n.public_key.size == 32 ? n.public_key.bytes : nullptr, n.role,
                          warmProtectedCategory(n), nodeInfoLiteHasXeddsaSigned(&n));
         // Demotion drops the node from the header table, so drop its satellites
         // too (the eviction chokepoint) - they'd otherwise orphan until the next

test/test_nodedb_blocked/test_main.cpp

@@ -29,6 +29,7 @@ class NodeDBTestShim : public NodeDB
 
     // Read back the role + protected category the warm tier cached for a node.
     bool warmMeta(NodeNum n, uint8_t &role, uint8_t &prot) { return warmStore.lookupMeta(n, role, prot); }
+    bool warmTake(NodeNum n, WarmNodeEntry &out) { return warmStore.take(n, out); }
 
     void clearHot()
     {
@@ -36,8 +37,13 @@ class NodeDBTestShim : public NodeDB
         numMeshNodes = 0;
     }
 
+    // The warm tier outlives setUp() (and a prior run's warm.dat), so a test that
+    // asserts on a warm row has to start from an empty one.
+    void clearWarm() { warmStore.clear(); }
+
+    // keySize < 32 seeds a partial key, as a truncated/short NodeInfo would leave behind.
     void push(NodeNum num, uint32_t lastHeard, bool favorite, bool ignored, bool withUser, bool withKey,
-              meshtastic_Config_DeviceConfig_Role role = meshtastic_Config_DeviceConfig_Role_CLIENT)
+              meshtastic_Config_DeviceConfig_Role role = meshtastic_Config_DeviceConfig_Role_CLIENT, pb_size_t keySize = 32)
     {
         meshtastic_NodeInfoLite n = meshtastic_NodeInfoLite_init_zero;
         n.num = num;
@@ -50,8 +56,8 @@ class NodeDBTestShim : public NodeDB
         if (withUser)
             nodeInfoLiteSetBit(&n, NODEINFO_BITFIELD_HAS_USER_MASK, true);
         if (withKey) {
-            n.public_key.size = 32;
-            memset(n.public_key.bytes, static_cast<uint8_t>(num & 0xff), 32);
+            n.public_key.size = keySize;
+            memset(n.public_key.bytes, static_cast<uint8_t>(num & 0xff), keySize);
             n.public_key.bytes[0] = 0x01; // ensure non-zero (all-zero == "no key")
         }
         meshNodes->push_back(n);
@@ -161,6 +167,38 @@ static void test_migration_carriesSignerBitThroughWarm(void)
     TEST_ASSERT_FALSE_MESSAGE(nodeInfoLiteHasXeddsaSigned(plainBack), "re-admission must not invent the signer bit");
 }
 
+// A warm record stores 32 raw key bytes with no length, so a partial hot-store key would be
+// indistinguishable from a real one once demoted. It must land as a keyless placeholder instead.
+static void test_migration_dropsShortKeyOnDemotion(void)
+{
+    db->clearWarm();
+    db->seedSelf();
+    const NodeNum shortKeyNum = 2000 + 3;
+    const NodeNum fullKeyNum = 2000 + 4;
+    const int extra = MAX_NUM_NODES + 30; // overflow so the oldest non-protected are demoted
+    // Warm entries steal the low 7 bits of last_heard for role and protected-category metadata
+    // (WARM_TIME_MASK), so seed multiples of 128 to keep the values representable once demoted.
+    for (int i = 1; i <= extra; i++)
+        db->push(2000 + i, /*last_heard=*/(uint32_t)i * 128, /*favorite=*/false, /*ignored=*/false, /*withUser=*/true,
+                 /*withKey=*/true, meshtastic_Config_DeviceConfig_Role_CLIENT,
+                 /*keySize=*/(NodeNum)(2000 + i) == shortKeyNum ? 31 : 32);
+
+    db->runDemote();
+
+    // Both left the hot store; only the full key is allowed through to the warm tier.
+    TEST_ASSERT_NULL(db->getMeshNode(shortKeyNum));
+    TEST_ASSERT_NULL(db->getMeshNode(fullKeyNum));
+    TEST_ASSERT_FALSE_MESSAGE(warmHasKey(shortKeyNum), "a 31-byte key must not be demoted as if it were a full key");
+    TEST_ASSERT_TRUE_MESSAGE(warmHasKey(fullKeyNum), "a full 32-byte key still survives demotion");
+
+    // The short-key node is still held, just keyless, so re-admission restores its last_heard.
+    uint8_t role = 0xFF, prot = 0xFF;
+    TEST_ASSERT_TRUE_MESSAGE(db->warmMeta(shortKeyNum, role, prot), "keyless placeholder row must still be present");
+    WarmNodeEntry placeholder = {};
+    TEST_ASSERT_TRUE_MESSAGE(db->warmTake(shortKeyNum, placeholder), "placeholder must be readable from the warm tier");
+    TEST_ASSERT_EQUAL_UINT32_MESSAGE(3u * 128, warmTimeOf(placeholder), "the keyless placeholder must carry last_heard");
+}
+
 // Favourite handling: a favourite is never the eviction victim, even when it is
 // the oldest node in a full hot store.
 static void test_eviction_preservesFavorite(void)
@@ -290,6 +328,7 @@ NDB_TEST_ENTRY void setup()
     RUN_TEST(test_migration_demotesOldestKeepsKeepersAndSelf);
     RUN_TEST(test_migration_carriesRoleAndProtectedIntoWarm);
     RUN_TEST(test_migration_carriesSignerBitThroughWarm);
+    RUN_TEST(test_migration_dropsShortKeyOnDemotion);
     RUN_TEST(test_eviction_preservesFavorite);
     RUN_TEST(test_eviction_prefersCurrentBootStampOverPost2038Epoch);
     RUN_TEST(test_ignored_survivesEvictionAndCleanup);