fix(ble): keep device addresses out of logs (#7478)

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)
         }

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}"
 }

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()

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")

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
             }