From cab39408df72669373bbfdeb328e84a2fe340fa0 Mon Sep 17 00:00:00 2001 From: James Rich <2199651+jamesarich@users.noreply.github.com> Date: Fri, 6 Feb 2026 13:55:20 -0600 Subject: [PATCH] refactor(node): Improve public key conflict handling (#4486) Signed-off-by: James Rich <2199651+jamesarich@users.noreply.github.com> --- .../core/database/dao/NodeInfoDaoTest.kt | 78 +++++++++++++++++-- .../core/database/dao/NodeInfoDao.kt | 50 +++++++----- .../core/ui/component/EditBase64Preference.kt | 10 ++- .../core/ui/component/NodeKeyStatusIcon.kt | 26 +++++-- .../node/component/NodeDetailsSection.kt | 27 +++++-- 5 files changed, 153 insertions(+), 38 deletions(-) diff --git a/core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoTest.kt b/core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoTest.kt index 2a47f4e43..0571e6396 100644 --- a/core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoTest.kt +++ b/core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoTest.kt @@ -22,6 +22,7 @@ import androidx.test.platform.app.InstrumentationRegistry import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map import kotlinx.coroutines.runBlocking +import okio.ByteString import okio.ByteString.Companion.toByteString import org.junit.After import org.junit.Assert.assertEquals @@ -338,10 +339,77 @@ class NodeInfoDaoTest { @Test fun testPkcMismatch() = runBlocking { - val newNode = testNodes[1].copy(user = testNodes[1].user.copy(public_key = ByteArray(32) { 99 }.toByteString())) - nodeInfoDao.putAll(listOf(newNode)) - val nodes = getNodes() - val containsMismatchNode = nodes.any { it.mismatchKey } - assertTrue(containsMismatchNode) + // First, ensure the node is in the DB with Key A + val nodeA = testNodes[10].copy(publicKey = ByteArray(32) { 1 }.toByteString()) + nodeInfoDao.upsert(nodeA) + + // Now upsert with Key B (mismatch) + val nodeB = + nodeA.copy( + publicKey = ByteArray(32) { 2 }.toByteString(), + user = nodeA.user.copy(public_key = ByteArray(32) { 2 }.toByteString()), + ) + nodeInfoDao.upsert(nodeB) + + val stored = nodeInfoDao.getNodeByNum(nodeA.num)!!.node + assertEquals(NodeEntity.ERROR_BYTE_STRING, stored.publicKey) + assertTrue(stored.toModel().mismatchKey) + } + + @Test + fun testRoutineUpdatePreservesKey() = runBlocking { + // 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)) + nodeInfoDao.upsert(nodeA) + + // Now upsert with an empty key (common in position/telemetry updates) + val nodeEmpty = nodeA.copy(publicKey = null, user = nodeA.user.copy(public_key = ByteString.EMPTY)) + nodeInfoDao.upsert(nodeEmpty) + + val stored = nodeInfoDao.getNodeByNum(nodeA.num)!!.node + assertEquals(keyA, stored.publicKey) + assertFalse(stored.toModel().mismatchKey) + } + + @Test + fun testRecoveryFromErrorState() = runBlocking { + // Start in Error state + val nodeError = + testNodes[10].copy( + publicKey = NodeEntity.ERROR_BYTE_STRING, + user = testNodes[10].user.copy(public_key = NodeEntity.ERROR_BYTE_STRING), + ) + nodeInfoDao.doUpsert(nodeError) + assertTrue(nodeInfoDao.getNodeByNum(nodeError.num)!!.toModel().mismatchKey) + + // Now upsert with a valid Key C + val keyC = ByteArray(32) { 3 }.toByteString() + val nodeC = nodeError.copy(publicKey = keyC, user = nodeError.user.copy(public_key = keyC)) + nodeInfoDao.upsert(nodeC) + + val stored = nodeInfoDao.getNodeByNum(nodeError.num)!!.node + assertEquals(keyC, stored.publicKey) + assertFalse(stored.toModel().mismatchKey) + } + + @Test + fun testLicensedUserClearsKey() = runBlocking { + // 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)) + nodeInfoDao.upsert(nodeA) + + // Upsert as licensed user + val nodeLicensed = + nodeA.copy( + user = nodeA.user.copy(is_licensed = true, public_key = ByteString.EMPTY), + publicKey = ByteString.EMPTY, + ) + nodeInfoDao.upsert(nodeLicensed) + + val stored = nodeInfoDao.getNodeByNum(nodeA.num)!!.node + assertTrue(stored.publicKey == null || (stored.publicKey?.size ?: 0) == 0) + assertFalse(stored.toModel().mismatchKey) } } diff --git a/core/database/src/main/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt b/core/database/src/main/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt index b11fd014c..a56abe304 100644 --- a/core/database/src/main/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt +++ b/core/database/src/main/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt @@ -84,6 +84,7 @@ interface NodeInfoDao { return newNode } + @Suppress("CyclomaticComplexMethod", "MagicNumber") private fun handleExistingNodeUpsertValidation(existingNode: NodeEntity, incomingNode: NodeEntity): NodeEntity { val isPlaceholder = incomingNode.user.hw_model == HardwareModel.UNSET val hasExistingUser = existingNode.user.hw_model != HardwareModel.UNSET @@ -113,27 +114,40 @@ interface NodeInfoDao { ) } - // A public key is considered matching if the incoming key equals the existing key, - // OR if the existing key is empty (allowing a new key to be set or an update to proceed). - val existingResolvedKey = existingNode.publicKey ?: existingNode.user.public_key - val isPublicKeyMatchingOrExistingIsEmpty = existingResolvedKey == incomingNode.publicKey || !existingNode.hasPKC + 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 - return if (isPublicKeyMatchingOrExistingIsEmpty) { - // Keys match or existing key was empty: trust the incoming node data completely. - // This allows for legitimate updates to user info and other fields. - incomingNode.copy(notes = resolvedNotes) - } else { - // Public key mismatch: This could be a factory reset or a hardware ID collision. - // We allow the name and user info to update, but we clear the public key - // to indicate that this node is no longer "verified" against the previous key. - incomingNode.copy( - user = incomingNode.user.copy(public_key = NodeEntity.ERROR_BYTE_STRING), - publicKey = NodeEntity.ERROR_BYTE_STRING, - notes = resolvedNotes, - ) - } + return incomingNode.copy( + user = incomingNode.user.copy(public_key = resolvedKey ?: ByteString.EMPTY), + publicKey = resolvedKey, + notes = resolvedNotes, + ) } @Query("SELECT * FROM my_node") diff --git a/core/ui/src/main/kotlin/org/meshtastic/core/ui/component/EditBase64Preference.kt b/core/ui/src/main/kotlin/org/meshtastic/core/ui/component/EditBase64Preference.kt index 497137e75..f5e6391e0 100644 --- a/core/ui/src/main/kotlin/org/meshtastic/core/ui/component/EditBase64Preference.kt +++ b/core/ui/src/main/kotlin/org/meshtastic/core/ui/component/EditBase64Preference.kt @@ -51,7 +51,7 @@ import org.meshtastic.core.strings.Res import org.meshtastic.core.strings.error import org.meshtastic.core.strings.reset -@Suppress("LongMethod") +@Suppress("LongMethod", "CyclomaticComplexMethod", "MagicNumber") @Composable fun EditBase64Preference( modifier: Modifier = Modifier, @@ -65,14 +65,16 @@ fun EditBase64Preference( onGenerateKey: (() -> Unit)? = null, trailingIcon: (@Composable () -> Unit)? = null, ) { - var valueState by remember { mutableStateOf(value.encodeToString()) } - val isError = value.encodeToString() != valueState + val isMismatch = value.size == 32 && value.toByteArray().all { it == 0.toByte() } + val errorString = stringResource(Res.string.error) + var valueState by remember { mutableStateOf(if (isMismatch) errorString else value.encodeToString()) } + val isError = value.encodeToString() != valueState || isMismatch // don't update values while the user is editing var isFocused by remember { mutableStateOf(false) } LaunchedEffect(value) { if (!isFocused) { - valueState = value.encodeToString() + valueState = if (isMismatch) errorString else value.encodeToString() } } diff --git a/core/ui/src/main/kotlin/org/meshtastic/core/ui/component/NodeKeyStatusIcon.kt b/core/ui/src/main/kotlin/org/meshtastic/core/ui/component/NodeKeyStatusIcon.kt index ef0da254b..07d256649 100644 --- a/core/ui/src/main/kotlin/org/meshtastic/core/ui/component/NodeKeyStatusIcon.kt +++ b/core/ui/src/main/kotlin/org/meshtastic/core/ui/component/NodeKeyStatusIcon.kt @@ -63,6 +63,7 @@ import org.meshtastic.core.strings.encryption_pkc import org.meshtastic.core.strings.encryption_pkc_text import org.meshtastic.core.strings.encryption_psk import org.meshtastic.core.strings.encryption_psk_text +import org.meshtastic.core.strings.error import org.meshtastic.core.strings.security_icon_help_dismiss import org.meshtastic.core.strings.security_icon_help_show_all import org.meshtastic.core.strings.security_icon_help_show_less @@ -170,6 +171,7 @@ enum class NodeKeySecurityState( ), } +@Suppress("LongMethod", "MagicNumber") @Composable private fun KeyStatusDialog(title: StringResource, text: StringResource, key: ByteString?, onDismiss: () -> Unit = {}) { var showAll by rememberSaveable { mutableStateOf(false) } @@ -190,16 +192,30 @@ private fun KeyStatusDialog(title: StringResource, text: StringResource, key: By Column(horizontalAlignment = Alignment.CenterHorizontally) { Text(text = stringResource(text), textAlign = TextAlign.Center) Spacer(Modifier.height(16.dp)) - if (key != null && title == Res.string.encryption_pkc) { - val keyString = Base64.encodeToString(key.toByteArray(), Base64.NO_WRAP) + if (key != null && (title == Res.string.encryption_pkc || title == Res.string.encryption_error)) { + val isMismatch = key.size == 32 && key.toByteArray().all { it == 0.toByte() } + val keyString = + if (isMismatch) { + stringResource(Res.string.error) + } else { + Base64.encodeToString(key.toByteArray(), Base64.NO_WRAP) + } Text( text = stringResource(Res.string.config_security_public_key) + ":", textAlign = TextAlign.Center, ) Spacer(Modifier.height(8.dp)) - SelectionContainer { Text(text = keyString, textAlign = TextAlign.Center) } - Spacer(Modifier.height(8.dp)) - CopyIconButton(valueToCopy = keyString, modifier = Modifier.padding(start = 8.dp)) + SelectionContainer { + Text( + text = keyString, + textAlign = TextAlign.Center, + color = if (isMismatch) MaterialTheme.colorScheme.error else Color.Unspecified, + ) + } + if (!isMismatch) { + Spacer(Modifier.height(8.dp)) + CopyIconButton(valueToCopy = keyString, modifier = Modifier.padding(start = 8.dp)) + } Spacer(Modifier.height(16.dp)) } } diff --git a/feature/node/src/main/kotlin/org/meshtastic/feature/node/component/NodeDetailsSection.kt b/feature/node/src/main/kotlin/org/meshtastic/feature/node/component/NodeDetailsSection.kt index 19de1f177..fbab30878 100644 --- a/feature/node/src/main/kotlin/org/meshtastic/feature/node/component/NodeDetailsSection.kt +++ b/feature/node/src/main/kotlin/org/meshtastic/feature/node/component/NodeDetailsSection.kt @@ -59,6 +59,7 @@ import org.meshtastic.core.strings.copy import org.meshtastic.core.strings.details import org.meshtastic.core.strings.encryption_error import org.meshtastic.core.strings.encryption_error_text +import org.meshtastic.core.strings.error import org.meshtastic.core.strings.hops_away import org.meshtastic.core.strings.node_id import org.meshtastic.core.strings.node_number @@ -289,11 +290,18 @@ private fun MqttAndVerificationRow(node: Node) { } @OptIn(ExperimentalFoundationApi::class) +@Suppress("LongMethod", "MagicNumber") @Composable private fun PublicKeyItem(publicKeyBytes: ByteArray) { val clipboard: Clipboard = LocalClipboard.current val coroutineScope = rememberCoroutineScope() - val publicKeyBase64 = Base64.encodeToString(publicKeyBytes, Base64.DEFAULT).trim() + val isMismatch = publicKeyBytes.all { it == 0.toByte() } && publicKeyBytes.size == 32 + val publicKeyBase64 = + if (isMismatch) { + stringResource(Res.string.error) + } else { + Base64.encodeToString(publicKeyBytes, Base64.DEFAULT).trim() + } val label = stringResource(Res.string.public_key) val copyLabel = stringResource(Res.string.copy) @@ -303,8 +311,10 @@ private fun PublicKeyItem(publicKeyBytes: ByteArray) { .defaultMinSize(minHeight = 48.dp) .combinedClickable( onLongClick = { - coroutineScope.launch { - clipboard.setClipEntry(ClipEntry(ClipData.newPlainText(label, publicKeyBase64))) + if (!isMismatch) { + coroutineScope.launch { + clipboard.setClipEntry(ClipEntry(ClipData.newPlainText(label, publicKeyBase64))) + } } }, onLongClickLabel = copyLabel, @@ -319,13 +329,18 @@ private fun PublicKeyItem(publicKeyBytes: ByteArray) { imageVector = MeshtasticIcons.Lock, contentDescription = null, modifier = Modifier.size(14.dp), - tint = MaterialTheme.colorScheme.primary.copy(alpha = 0.8f), + tint = + if (isMismatch) { + MaterialTheme.colorScheme.error + } else { + MaterialTheme.colorScheme.primary.copy(alpha = 0.8f) + }, ) Spacer(Modifier.width(6.dp)) Text( text = label, style = MaterialTheme.typography.labelSmall, - color = MaterialTheme.colorScheme.onSurfaceVariant, + color = if (isMismatch) MaterialTheme.colorScheme.error else MaterialTheme.colorScheme.onSurfaceVariant, fontWeight = FontWeight.Bold, ) } @@ -333,7 +348,7 @@ private fun PublicKeyItem(publicKeyBytes: ByteArray) { Text( text = publicKeyBase64, style = MaterialTheme.typography.bodySmall.copy(fontFamily = FontFamily.Monospace), - color = MaterialTheme.colorScheme.onSurface, + color = if (isMismatch) MaterialTheme.colorScheme.error else MaterialTheme.colorScheme.onSurface, ) } }