feat(db): enhance public key conflict handling (#4555)

Signed-off-by: James Rich <2199651+jamesarich@users.noreply.github.com>

AGENTS.md

@@ -103,6 +103,7 @@ This file serves as a comprehensive guide for AI agents and developers working o
 -   **Protobuf:** Communication with the device uses Protobufs. The definitions are in `core/proto`. This is a Git submodule, but the build system handles it.
 -   **Legacy:** Some code in `app/` uses the `com.geeksville.mesh` package. Newer code in `core/` and `feature/` uses `org.meshtastic.*`. Respect the existing package structure of the file you are editing.
 -   **Versioning:** Do not manually edit `versionCode` or `versionName`. These are managed by the build system and CI/CD.
+-   **Database Safety:** When modifying critical database logic (e.g., `NodeInfoDao`), always ensure you have explicit test coverage for security edge cases (like PKC conflicts or key wiping). Refer to `core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoTest.kt` for examples.
 
 ## 7. Troubleshooting
 

core/database/README.md

@@ -1,5 +1,25 @@
 # `:core:database`
 
+This module provides the local Room database persistence layer for the application.
+
+## Key Components
+
+-   **`MeshtasticDatabase`**: The main Room database class.
+-   **DAOs (Data Access Objects)**:
+    -   `NodeInfoDao`: Manages storage and retrieval of node information (`NodeEntity`). Contains critical logic for handling Public Key Conflict (PKC) resolution and preventing identity wiping attacks.
+    -   `PacketDao`: Handles storage of mesh packets.
+    -   `ChatMessageDao`: Manages chat message history.
+-   **Entities**:
+    -   `NodeEntity`: Represents a node on the mesh.
+    -   `PacketEntity`: Represents a stored packet.
+
+## Security Considerations
+
+### Public Key Conflict (PKC) Handling
+The `NodeInfoDao` implements specific logic to protect against impersonation and "wipe" attacks:
+-   **Wipe Protection**: Receiving an `is_licensed=true` packet (which normally clears the public key for compliance) will **not** clear an existing valid public key if one is already known. This prevents attackers from sending fake licensed packets to wipe keys from the DB.
+-   **Conflict Detection**: If a new key arrives for an existing node ID that conflicts with a known valid key, the key is set to `ERROR_BYTE_STRING` to flag the potential impersonation.
+
 ## Module dependency graph
 
 <!--region graph-->

core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoTest.kt

@@ -38,7 +38,6 @@ import org.meshtastic.core.database.model.Node
 import org.meshtastic.core.database.model.NodeSortOption
 import org.meshtastic.core.model.util.onlineTimeThreshold
 import org.meshtastic.proto.HardwareModel
-import org.meshtastic.proto.Position
 import org.meshtastic.proto.User
 
 @RunWith(AndroidJUnit4::class)
@@ -256,14 +255,14 @@ class NodeInfoDaoTest {
     @Test
     fun testSortByAlpha() = runBlocking {
         val nodes = getNodes(sort = NodeSortOption.ALPHABETICAL)
-        val sortedNodes = nodes.sortedBy { it.user.long_name?.uppercase() ?: "" }
+        val sortedNodes = nodes.sortedBy { it.user.long_name.uppercase() }
         assertEquals(sortedNodes, nodes)
     }
 
     @Test
     fun testSortByDistance() = runBlocking {
         val nodes = getNodes(sort = NodeSortOption.DISTANCE)
-        fun NodeEntity.toNode() = Node(num = num, user = user, position = position ?: Position())
+        fun NodeEntity.toNode() = Node(num = num, user = user, position = position)
         val sortedNodes =
             nodes.sortedWith( // nodes with invalid (null) positions at the end
                 compareBy<Node> { it.validPosition == null }.thenBy { it.distance(ourNode.toNode()) },
@@ -281,7 +280,7 @@ class NodeInfoDaoTest {
     @Test
     fun testSortByViaMqtt() = runBlocking {
         val nodes = getNodes(sort = NodeSortOption.VIA_MQTT)
-        val sortedNodes = nodes.sortedBy { it.user.long_name?.contains("(MQTT)") == true }
+        val sortedNodes = nodes.sortedBy { it.user.long_name.contains("(MQTT)") }
         assertEquals(sortedNodes, nodes)
     }
 
@@ -339,8 +338,14 @@ class NodeInfoDaoTest {
 
     @Test
     fun testPkcMismatch() = runBlocking {
+        val newNodeNum = 9999
         // First, ensure the node is in the DB with Key A
-        val nodeA = testNodes[10].copy(publicKey = ByteArray(32) { 1 }.toByteString())
+        val nodeA =
+            testNodes[0].copy(
+                num = newNodeNum,
+                publicKey = ByteArray(32) { 1 }.toByteString(),
+                user = testNodes[0].user.copy(id = "!uniqueId1", public_key = ByteArray(32) { 1 }.toByteString()),
+            )
         nodeInfoDao.upsert(nodeA)
 
         // Now upsert with Key B (mismatch)
@@ -358,9 +363,15 @@ class NodeInfoDaoTest {
 
     @Test
     fun testRoutineUpdatePreservesKey() = runBlocking {
+        val newNodeNum = 9998
         // First, ensure the node is in the DB with Key A
         val keyA = ByteArray(32) { 1 }.toByteString()
-        val nodeA = testNodes[10].copy(publicKey = keyA, user = testNodes[10].user.copy(public_key = keyA))
+        val nodeA =
+            testNodes[0].copy(
+                num = newNodeNum,
+                publicKey = keyA,
+                user = testNodes[0].user.copy(id = "!uniqueId2", public_key = keyA),
+            )
         nodeInfoDao.upsert(nodeA)
 
         // Now upsert with an empty key (common in position/telemetry updates)
@@ -374,11 +385,13 @@ class NodeInfoDaoTest {
 
     @Test
     fun testRecoveryFromErrorState() = runBlocking {
+        val newNodeNum = 9997
         // Start in Error state
         val nodeError =
-            testNodes[10].copy(
+            testNodes[0].copy(
+                num = newNodeNum,
                 publicKey = NodeEntity.ERROR_BYTE_STRING,
-                user = testNodes[10].user.copy(public_key = NodeEntity.ERROR_BYTE_STRING),
+                user = testNodes[0].user.copy(id = "!uniqueId3", public_key = NodeEntity.ERROR_BYTE_STRING),
             )
         nodeInfoDao.doUpsert(nodeError)
         assertTrue(nodeInfoDao.getNodeByNum(nodeError.num)!!.toModel().mismatchKey)
@@ -394,13 +407,19 @@ class NodeInfoDaoTest {
     }
 
     @Test
-    fun testLicensedUserClearsKey() = runBlocking {
+    fun testLicensedUserDoesNotClearKey() = runBlocking {
+        val newNodeNum = 9996
         // Start with a key
         val keyA = ByteArray(32) { 1 }.toByteString()
-        val nodeA = testNodes[10].copy(publicKey = keyA, user = testNodes[10].user.copy(public_key = keyA))
+        val nodeA =
+            testNodes[0].copy(
+                num = newNodeNum,
+                publicKey = keyA,
+                user = testNodes[0].user.copy(id = "!uniqueId4", public_key = keyA),
+            )
         nodeInfoDao.upsert(nodeA)
 
-        // Upsert as licensed user
+        // Upsert as licensed user (without key)
         val nodeLicensed =
             nodeA.copy(
                 user = nodeA.user.copy(is_licensed = true, public_key = ByteString.EMPTY),
@@ -409,7 +428,72 @@ class NodeInfoDaoTest {
         nodeInfoDao.upsert(nodeLicensed)
 
         val stored = nodeInfoDao.getNodeByNum(nodeA.num)!!.node
-        assertTrue(stored.publicKey == null || (stored.publicKey?.size ?: 0) == 0)
+        // Should NOT clear key to prevent PKC wipe attack
+        assertEquals(keyA, stored.publicKey)
         assertFalse(stored.toModel().mismatchKey)
     }
+
+    @Test
+    fun testValidLicensedUserNoKey() = runBlocking {
+        val newNodeNum = 9995
+        // Start with no key and licensed status
+        val nodeLicensed =
+            testNodes[0].copy(
+                num = newNodeNum,
+                publicKey = null,
+                user = testNodes[0].user.copy(id = "!uniqueId5", is_licensed = true, public_key = ByteString.EMPTY),
+            )
+        nodeInfoDao.upsert(nodeLicensed)
+
+        val stored = nodeInfoDao.getNodeByNum(newNodeNum)!!.node
+        assertEquals(ByteString.EMPTY, stored.publicKey)
+        assertFalse(stored.toModel().mismatchKey)
+    }
+
+    @Test
+    fun testPlaceholderUpdatePreservesIdentity() = runBlocking {
+        val newNodeNum = 9994
+        val keyA = ByteArray(32) { 5 }.toByteString()
+        val originalName = "Real Name"
+        // 1. Create a full node with key and name
+        val fullNode =
+            testNodes[0].copy(
+                num = newNodeNum,
+                longName = originalName,
+                publicKey = keyA,
+                user =
+                testNodes[0]
+                    .user
+                    .copy(
+                        id = "!uniqueId6",
+                        long_name = originalName,
+                        public_key = keyA,
+                        hw_model = HardwareModel.TLORA_V2, // Set a specific HW model
+                    ),
+            )
+        nodeInfoDao.upsert(fullNode)
+
+        // 2. Simulate receiving a placeholder packet (e.g. from a legacy node or partial info)
+        // HW Model UNSET, Default Name "Meshtastic XXXX"
+        val placeholderNode =
+            fullNode.copy(
+                user =
+                fullNode.user.copy(
+                    hw_model = HardwareModel.UNSET,
+                    long_name = "Meshtastic 1234",
+                    public_key = ByteString.EMPTY,
+                ),
+                longName = "Meshtastic 1234",
+                publicKey = null,
+            )
+        nodeInfoDao.upsert(placeholderNode)
+
+        // 3. Verify that the identity (Name and Key) is preserved
+        val stored = nodeInfoDao.getNodeByNum(newNodeNum)!!.node
+        assertEquals(originalName, stored.longName)
+        assertEquals(keyA, stored.publicKey)
+        // Ensure HW model is NOT overwritten by UNSET if we preserve the user
+        // Note: The logic in handleExistingNodeUpsertValidation copies the *existing* user back.
+        assertEquals(HardwareModel.TLORA_V2, stored.user.hw_model)
+    }
 }

core/database/src/main/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt

@@ -35,6 +35,10 @@ import org.meshtastic.proto.HardwareModel
 @Dao
 interface NodeInfoDao {
 
+    companion object {
+        const val KEY_SIZE = 32
+    }
+
     /**
      * Verifies a [NodeEntity] before an upsert operation. It handles populating the publicKey for lazy migration,
      * checks for public key conflicts with new nodes, and manages updates to existing nodes, particularly in cases of
@@ -84,64 +88,75 @@ interface NodeInfoDao {
         return newNode
     }
 
+    /**
+     * Resolves the public key for an existing node during an update.
+     *
+     * This function implements safety checks to prevent public key conflicts (PKC) and ensure robust handling of key
+     * updates.
+     *
+     * @param existingNode The current state of the node in the database.
+     * @param incomingNode The new node data being upserted.
+     * @return The resolved [ByteString] for the public key:
+     * - [NodeEntity.ERROR_BYTE_STRING]: If there is a mismatch between a valid existing key and a new incoming key.
+     * - `incomingNode.publicKey`: If the incoming key is new, matches the existing one, or if recovering from an error
+     *   state.
+     * - `existingNode.publicKey`: If the incoming update has no key, or if the user is licensed but already has a valid
+     *   key (prevents wiping).
+     * - [ByteString.EMPTY]: If the user is licensed and didn't previously have a key (or if key is explicitly cleared).
+     */
+    private fun resolvePublicKey(existingNode: NodeEntity, incomingNode: NodeEntity): ByteString? {
+        val existingKey = existingNode.publicKey ?: existingNode.user.public_key
+        val incomingKey = incomingNode.publicKey
+
+        val incomingHasKey = (incomingKey?.size ?: 0) == KEY_SIZE
+        val existingHasKey = (existingKey?.size ?: 0) == KEY_SIZE && existingKey != NodeEntity.ERROR_BYTE_STRING
+
+        return when {
+            incomingHasKey -> {
+                if (existingHasKey && incomingKey != existingKey) {
+                    // Actual mismatch between two non-empty keys
+                    NodeEntity.ERROR_BYTE_STRING
+                } else {
+                    // New key, same key, or recovery from Error state
+                    incomingKey
+                }
+            }
+            existingHasKey -> existingKey
+            incomingNode.user.is_licensed -> ByteString.EMPTY
+            else -> existingKey
+        }
+    }
+
+    /**
+     * Handles the validation logic when upserting an existing node.
+     *
+     * It distinguishes between two scenarios:
+     * 1. **Preservation**: If the incoming update is a placeholder (unset HW model) with a default name, and the
+     *    existing node has full user info, we preserve the existing identity (user, keys, names, verification) while
+     *    updating telemetry and status fields from the incoming packet.
+     * 2. **Update**: If it's a normal update, we validate the public key using [resolvePublicKey] to prevent conflicts
+     *    or accidental key wiping, and then update the node.
+     */
     @Suppress("CyclomaticComplexMethod", "MagicNumber")
     private fun handleExistingNodeUpsertValidation(existingNode: NodeEntity, incomingNode: NodeEntity): NodeEntity {
+        val resolvedNotes = incomingNode.notes.ifBlank { existingNode.notes }
+
         val isPlaceholder = incomingNode.user.hw_model == HardwareModel.UNSET
         val hasExistingUser = existingNode.user.hw_model != HardwareModel.UNSET
         val isDefaultName = incomingNode.user.long_name?.matches(Regex("^Meshtastic [0-9a-fA-F]{4}$")) == true
 
-        val shouldPreserve = hasExistingUser && isPlaceholder && isDefaultName
-
-        if (shouldPreserve) {
-            // Preserve existing name and user info, but update metadata like lastHeard, SNR, and position.
-            val resolvedNotes = if (incomingNode.notes.isBlank()) existingNode.notes else incomingNode.notes
-            return existingNode.copy(
-                lastHeard = incomingNode.lastHeard,
-                snr = incomingNode.snr,
-                rssi = incomingNode.rssi,
-                position = incomingNode.position,
-                hopsAway = incomingNode.hopsAway,
-                deviceTelemetry = incomingNode.deviceTelemetry,
-                environmentTelemetry = incomingNode.environmentTelemetry,
-                powerTelemetry = incomingNode.powerTelemetry,
-                paxcounter = incomingNode.paxcounter,
-                channel = incomingNode.channel,
-                viaMqtt = incomingNode.viaMqtt,
-                isFavorite = incomingNode.isFavorite,
-                isIgnored = incomingNode.isIgnored,
-                isMuted = incomingNode.isMuted,
+        if (hasExistingUser && isPlaceholder && isDefaultName) {
+            return incomingNode.copy(
+                user = existingNode.user,
+                publicKey = existingNode.publicKey,
+                longName = existingNode.longName,
+                shortName = existingNode.shortName,
+                manuallyVerified = existingNode.manuallyVerified,
                 notes = resolvedNotes,
             )
         }
 
-        val existingKey = existingNode.publicKey ?: existingNode.user.public_key
-        val incomingKey = incomingNode.publicKey
-
-        val incomingHasKey = (incomingKey?.size ?: 0) == 32
-        val existingHasKey = (existingKey?.size ?: 0) == 32 && existingKey != NodeEntity.ERROR_BYTE_STRING
-
-        val resolvedKey =
-            when {
-                incomingHasKey -> {
-                    if (existingHasKey && incomingKey != existingKey) {
-                        // Actual mismatch between two non-empty keys
-                        NodeEntity.ERROR_BYTE_STRING
-                    } else {
-                        // New key, same key, or recovery from Error state
-                        incomingKey
-                    }
-                }
-                incomingNode.user.is_licensed -> {
-                    // Explicitly clear key for licensed (HAM) users
-                    ByteString.EMPTY
-                }
-                else -> {
-                    // Routine update without key: preserve what we have (even if it's currently Error)
-                    existingKey
-                }
-            }
-
-        val resolvedNotes = if (incomingNode.notes.isBlank()) existingNode.notes else incomingNode.notes
+        val resolvedKey = resolvePublicKey(existingNode, incomingNode)
 
         return incomingNode.copy(
             user = incomingNode.user.copy(public_key = resolvedKey ?: ByteString.EMPTY),