From 3bb9c7f2c97f4d6836a903917afa3f41f01cf909 Mon Sep 17 00:00:00 2001 From: James Rich <2199651+jamesarich@users.noreply.github.com> Date: Wed, 30 Sep 2026 03:20:04 +0000 Subject: [PATCH] fix(ble): keep device addresses out of logs (#7478) --- .../meshtastic/core/ble/KableBleConnection.kt | 2 +- .../core/konsist/BleAddressLoggingTest.kt | 136 ++++++++++++------ .../core/network/radio/BleRadioTransport.kt | 4 +- .../network/radio/BleRadioTransportTest.kt | 24 ++++ .../feature/discovery/DiscoveryScanEngine.kt | 5 +- 5 files changed, 126 insertions(+), 45 deletions(-) diff --git a/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/KableBleConnection.kt b/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/KableBleConnection.kt index 4b7e5fc1c0..bee3fe0567 100644 --- a/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/KableBleConnection.kt +++ b/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/KableBleConnection.kt @@ -154,7 +154,7 @@ class KableBleConnection(private val scope: CoroutineScope, private val loggingC // _deviceFlow.emit() is intentionally outside this block — making it // non-cancellable could hang teardown on a slow collector. withContext(NonCancellable) { - cleanUpPeripheral(device.address) + cleanUpPeripheral(tag = device.address.anonymize()) peripheral = p ActiveBleConnection.active = ActiveConnection(p, device.address) } diff --git a/core/konsist/src/jvmTest/kotlin/org/meshtastic/core/konsist/BleAddressLoggingTest.kt b/core/konsist/src/jvmTest/kotlin/org/meshtastic/core/konsist/BleAddressLoggingTest.kt index 93e1bc29e5..29c6c9d47f 100644 --- a/core/konsist/src/jvmTest/kotlin/org/meshtastic/core/konsist/BleAddressLoggingTest.kt +++ b/core/konsist/src/jvmTest/kotlin/org/meshtastic/core/konsist/BleAddressLoggingTest.kt @@ -17,6 +17,7 @@ package org.meshtastic.core.konsist import com.lemonappdev.konsist.api.Konsist +import com.lemonappdev.konsist.api.declaration.KoFileDeclaration import kotlin.test.Test import kotlin.test.assertTrue @@ -25,9 +26,9 @@ import kotlin.test.assertTrue * Datadog and Crashlytics on analytics the user is opted into by default. So an address must never be interpolated into * log or exception text raw — it goes through `Any?.anonymize()`, which keeps only a short suffix. * - * This is enforced as an architecture rule rather than by review because the failure mode is missing a site: a previous - * attempt anonymised the hand-written log statements in `core/ble` and missed the Kable `identifier`, which stamps the - * address onto *every* line the BLE library emits, plus further sites in the DFU transports and WiFi provisioning. + * This is an architecture rule rather than a review item because the failure mode is a missed site, and the easiest + * sites to miss reach the log indirectly: a Kable logging `identifier`, which stamps the address onto every line the + * BLE library emits, or a `tag` that a helper such as `retryBleOperation` prefixes to its own lines. * * Scoped to the BLE-adjacent modules so matching on the `address` suffix stays low-noise. That scope includes the * transport modules, so TCP hosts go through `anonymizePublicHost()`, which keeps a host on the user's own network @@ -43,6 +44,7 @@ class BleAddressLoggingTest { "/feature/firmware/", "/feature/wifi-provision/", "/feature/connections/", + "/feature/discovery/", "/androidApp/", "/desktopApp/", ) @@ -55,12 +57,29 @@ class BleAddressLoggingTest { /** * Files whose `address` names hardware, not a person. The Android serial transport's address is the USB - * vendor-product pair (`usbSerialStableKey()`), which identifies the chip model. + * vendor-product pair (`usbSerialStableKey()`), which identifies the chip model, and the firmware retriever's are + * UF2 flash offsets. */ - private val notPersonalAddressFiles = listOf("SerialRadioTransport.kt") + private val notPersonalAddressFiles = listOf("SerialRadioTransport.kt", "FirmwareRetriever.kt") - /** Interpolation of anything ending in `address`, e.g. `${device.address}` or `$address`. */ - private val interpolatedAddress = Regex("""\$\{?[A-Za-z0-9_.]*[aA]ddress}?""") + /** + * Start of a call whose text reaches a log or a crash report. A `Logger.withTag(...)` prefix is part of the start. + */ + private val diagnosticCallStart = + Regex( + """Logger(\.withTag\([^)]*\))?\.\w+|\bthrow\s+\w+\s*\(|""" + + """\b(error|check|require|checkNotNull|requireNotNull|println)\s*\(""", + ) + + /** A string template entry, `${...}` or `$name`. */ + private val interpolation = Regex("""\$\{[^}]*}|\$[A-Za-z_]\w*""") + + private val addressReference = Regex("""[aA]ddress\b""") + + /** A named log-tag argument such as `tag = address` or Kable's `identifier = ...`, or a `withTag(...)` argument. */ + private val logTagArgument = Regex("""\b(tag|logTag|identifier)\s*=(?!=)\s*([^,)\n]*)|withTag\(([^)\n]*)\)""") + + private val stringLiteral = Regex("\"(?:\\\\.|[^\"\\\\])*\"") /** * Files this rule covers. @@ -80,7 +99,14 @@ class BleAddressLoggingTest { val paths = scannedFiles().map { it.scanPath } assertTrue(paths.isNotEmpty(), emptyScanMessage("BLE-scoped scan")) - for (file in listOf("KableBleConnection.kt", "BleRadioTransport.kt", "SharedRadioInterfaceService.kt")) { + val expected = + listOf( + "KableBleConnection.kt", + "BleRadioTransport.kt", + "SharedRadioInterfaceService.kt", + "DiscoveryScanEngine.kt", + ) + for (file in expected) { assertTrue( paths.any { it.endsWith(file) }, "expected $file in scope; got ${paths.size} files, e.g. ${paths.take(3)}", @@ -92,12 +118,33 @@ class BleAddressLoggingTest { fun `a BLE address is never interpolated into log or exception text without anonymize`() { val offenders = scannedFiles().flatMap { file -> - file.text.lines().withIndex().mapNotNull { (index, line) -> - val isDiagnostic = "Logger." in line || "throw " in line || "check(" in line || "require(" in line - val interpolates = interpolatedAddress.containsMatchIn(line) - val anonymised = "anonymize" in line - if (isDiagnostic && interpolates && !anonymised) { - "${file.scanPath.substringAfterLast("/kotlin/")}:${index + 1}: " + line.trim() + diagnosticCallStart.findAll(file.text).flatMap { call -> + interpolation + .findAll(callText(file.text, call)) + .map { it.value } + .filter { addressReference.containsMatchIn(it) && "anonymize" !in it } + .map { "${file.location(call.range.first)}: ${call.value} ... $it" } + } + } + + assertTrue( + offenders.isEmpty(), + "BLE addresses must be anonymised in diagnostic text. Offending calls:\n" + offenders.joinToString("\n"), + ) + } + + @Test + fun `a BLE address is never used as a log tag without anonymize`() { + val offenders = + scannedFiles().flatMap { file -> + logTagArgument.findAll(file.text).mapNotNull { argument -> + val value = argument.groupValues[2].ifEmpty { argument.groupValues[3] } + val expression = + stringLiteral.replace(value) { literal -> + interpolation.findAll(literal.value).joinToString(" ") { it.value } + } + if (addressReference.containsMatchIn(expression) && "anonymize" !in expression) { + "${file.location(argument.range.first)}: ${argument.value.trim()}" } else { null } @@ -106,34 +153,43 @@ class BleAddressLoggingTest { assertTrue( offenders.isEmpty(), - "BLE addresses must be anonymised in diagnostic text. Offending lines:\n" + offenders.joinToString("\n"), + "A log tag or Kable logging identifier must be anonymised. Offending arguments:\n" + + offenders.joinToString("\n"), ) } - /** - * Kable stamps its `Logging.identifier` onto every line it emits, so passing a raw address there leaks it from - * library-internal logging that no per-call-site review would catch. - */ - @Test - fun `the Kable logging identifier is never a raw address`() { - val offenders = - Konsist.scopeFromProject() - .files - .filterNot { it.isNestedAgentWorktree() } - .filter { "/core/ble/" in it.scanPath } - .flatMap { file -> - file.text.lines().withIndex().mapNotNull { (index, line) -> - if ("identifier =" in line && "address" in line && "anonymize" !in line) { - "${file.scanPath.substringAfterLast("/kotlin/")}:${index + 1}: " + line.trim() - } else { - null - } - } - } - - assertTrue( - offenders.isEmpty(), - "Kable's logging identifier must be anonymised. Offending lines:\n" + offenders.joinToString("\n"), - ) + /** The text of the call starting at [start]: its argument list and its trailing lambda, each when present. */ + private fun callText(text: String, start: MatchResult): String { + var end = start.range.last + 1 + if (text[end - 1] == '(') { + end = closingIndex(text, end - 1) + 1 + } else { + val arguments = skipBlanks(text, end) + if (arguments < text.length && text[arguments] == '(') end = closingIndex(text, arguments) + 1 + } + val lambda = skipBlanks(text, end) + if (lambda < text.length && text[lambda] == '{') end = closingIndex(text, lambda) + 1 + return text.substring(start.range.first, end) } + + private fun skipBlanks(text: String, from: Int): Int { + var index = from + while (index < text.length && (text[index] == ' ' || text[index] == '\t')) index++ + return index + } + + /** Index of the bracket that closes the one at [open], or the last index of [text] when it never closes. */ + private fun closingIndex(text: String, open: Int): Int { + val openBracket = text[open] + val closeBracket = if (openBracket == '(') ')' else '}' + var depth = 0 + for (index in open until text.length) { + if (text[index] == openBracket) depth++ + if (text[index] == closeBracket && --depth == 0) return index + } + return text.lastIndex + } + + private fun KoFileDeclaration.location(offset: Int): String = + "${scanPath.substringAfterLast("/kotlin/")}:${text.take(offset).count { it == '\n' } + 1}" } diff --git a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt index da896f1cf2..12e3811adb 100644 --- a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt +++ b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt @@ -554,7 +554,7 @@ class BleRadioTransport( private suspend fun onConnected() { try { bleConnection.deviceFlow.first()?.let { device -> - val rssi = retryBleOperation(tag = address) { device.readRssi() } + val rssi = retryBleOperation(tag = address.anonymize()) { device.readRssi() } Logger.d { "[${address.anonymize()}] Connection confirmed. " + "Initial RSSI: ${rssi?.let { "$it dBm" } ?: "unknown"}" @@ -805,7 +805,7 @@ class BleRadioTransport( private suspend fun writePacket(session: BleSession, packet: ByteArray) { try { - retryBleOperation(tag = address, retryWhile = { activeSession.value === session }) { + retryBleOperation(tag = address.anonymize(), retryWhile = { activeSession.value === session }) { session.profile.sendToRadio(packet) } val sent = packetsSent.incrementAndGet() diff --git a/core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/BleRadioTransportTest.kt b/core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/BleRadioTransportTest.kt index 7cfe56cba5..5303d931e9 100644 --- a/core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/BleRadioTransportTest.kt +++ b/core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/BleRadioTransportTest.kt @@ -44,6 +44,7 @@ import org.meshtastic.core.ble.MeshtasticBleConstants.FROMRADIO_CHARACTERISTIC import org.meshtastic.core.ble.MeshtasticBleConstants.SERVICE_UUID import org.meshtastic.core.model.RadioNotConnectedException import org.meshtastic.core.repository.RadioInterfaceService +import org.meshtastic.core.testing.CapturingLogWriter import org.meshtastic.core.testing.FakeBleConnection import org.meshtastic.core.testing.FakeBleConnectionFactory import org.meshtastic.core.testing.FakeBleDevice @@ -195,6 +196,29 @@ class BleRadioTransportTest { } } + @Test + fun `a retried BLE write failure logs the device address only in anonymized form`() = runTest { + val device = FakeBleDevice(address = address, name = "Test Device") + bluetoothRepository.bond(device) + scanner.emitDevice(device) + val logs = CapturingLogWriter.install() + val bleTransport = bleTransportOn(this, FakeRadioInterfaceService()) + bleTransport.start() + + try { + advanceTimeBy(4_000L) + connection.service.writeException = RuntimeException("write rejected") + assertTrue(bleTransport.handleSendToRadio(byteArrayOf(1, 2, 3))) + advanceTimeBy(1_000L) + + assertTrue(logs.messages().any { "BLE operation failed" in it }, "the write retry path must have logged") + logs.assertNotLogged(address) + } finally { + bleTransport.close() + CapturingLogWriter.uninstall() + } + } + @Test fun `close drains every write admitted by the active BLE profile generation`() = runTest { val device = FakeBleDevice(address = address, name = "Test Device") diff --git a/feature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryScanEngine.kt b/feature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryScanEngine.kt index cc2ee93704..19d28c84e4 100644 --- a/feature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryScanEngine.kt +++ b/feature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryScanEngine.kt @@ -52,6 +52,7 @@ import org.meshtastic.core.model.ChannelOption import org.meshtastic.core.model.ConnectionState import org.meshtastic.core.model.DataPacket import org.meshtastic.core.model.numChannels +import org.meshtastic.core.model.util.anonymize import org.meshtastic.core.model.util.decodeOrNull import org.meshtastic.core.model.util.snrOrNull import org.meshtastic.core.repository.DiscoveryPacketCollector @@ -773,8 +774,8 @@ class DiscoveryScanEngine( // A null return means this device's session row is not in the active database and the dwell is unwritable. if (discoveryDao.insertDwellIfSessionExists(result, discoveredNodeEntities(), deviceAddress) == null) { Logger.w { - "DiscoveryScanEngine: session $sessionId for $deviceAddress is not in the active database; " + - "skipping dwell persistence" + "DiscoveryScanEngine: session $sessionId for ${deviceAddress.anonymize()} is not in the active " + + "database; skipping dwell persistence" } return@withLock }