From 59cd29ee2557beeda77acec67cfe23e1af67a326 Mon Sep 17 00:00:00 2001 From: Taksh Date: Mon, 24 Aug 2026 20:16:15 +0530 Subject: [PATCH 1/2] fix(ble): make the GATT broadcast subscription idempotent Writing the CCCD is the one GATT operation a peer can repeat freely on an open connection: no authentication, no Noise session, no announcement, and the Bluetooth spec puts no limit on how often a client may rewrite the descriptor. onDescriptorWriteRequest attached two non-idempotent effects to it. Each ENABLE_NOTIFICATION write appended to subscribedDevices with no contains() check, so N writes put N entries in the list one address owns. broadcastSinglePacketInternal iterates that list and notifies per entry, so every broadcast the mesh produces is then transmitted to that address N times. Each write also scheduled onDeviceConnected, which calls sendBroadcastAnnounce(). That is unthrottled and floods at maxTtl, so a peer could drive a mesh-wide ANNOUNCE at whatever rate it wrote the descriptor. Subscribe by address and return whether the device was newly subscribed, run the peer-connect side effects only on that transition, and honour DISABLE_NOTIFICATION_VALUE instead of keeping a peer on the broadcast feed until it disconnects. Removal now matches on address like the connection cleanup paths, so an unsubscribe cannot miss because the framework handed us a different BluetoothDevice instance for the same address. Refs #901 --- .../mesh/BluetoothConnectionTracker.kt | 33 ++++- .../mesh/BluetoothGattServerManager.kt | 27 +++- .../android/mesh/GattSubscriptionPolicy.kt | 34 +++++ .../mesh/GattBroadcastSubscriptionTest.kt | 129 ++++++++++++++++++ docs/device_manager.md | 8 ++ 5 files changed, 218 insertions(+), 13 deletions(-) create mode 100644 app/src/main/java/com/bitchat/android/mesh/GattSubscriptionPolicy.kt create mode 100644 app/src/test/kotlin/com/bitchat/android/mesh/GattBroadcastSubscriptionTest.kt diff --git a/app/src/main/java/com/bitchat/android/mesh/BluetoothConnectionTracker.kt b/app/src/main/java/com/bitchat/android/mesh/BluetoothConnectionTracker.kt index 94feb68d..7c8a2a26 100644 --- a/app/src/main/java/com/bitchat/android/mesh/BluetoothConnectionTracker.kt +++ b/app/src/main/java/com/bitchat/android/mesh/BluetoothConnectionTracker.kt @@ -171,18 +171,39 @@ class BluetoothConnectionTracker( } /** - * Add a subscribed device + * Subscribe a device to broadcast notifications. + * + * Idempotent by address: a peer may rewrite the notification descriptor as + * often as it likes, and each extra entry here would notify that address one + * more time for every broadcast the mesh produces. Returns true only when + * the device was not already subscribed, so callers can keep peer-connect + * side effects tied to an actual new subscription. */ - fun addSubscribedDevice(device: BluetoothDevice) { - subscribedDevices.add(device) - } + fun addSubscribedDevice(device: BluetoothDevice): Boolean = + synchronized(connectionStateLock) { + if (subscribedDevices.any { it.address == device.address }) { + false + } else { + subscribedDevices.add(device) + true + } + } /** - * Remove a subscribed device + * Remove a subscribed device. Matches on address, like the connection + * cleanup paths, so an entry cannot outlive an unsubscribe because the + * framework handed us a different BluetoothDevice instance for the same + * address. */ fun removeSubscribedDevice(device: BluetoothDevice) { - subscribedDevices.remove(device) + synchronized(connectionStateLock) { + subscribedDevices.removeAll { it.address == device.address } + } } + + /** Whether this address currently receives broadcast notifications. */ + fun isSubscribed(deviceAddress: String): Boolean = + subscribedDevices.any { it.address == deviceAddress } /** * Check if device is already connected diff --git a/app/src/main/java/com/bitchat/android/mesh/BluetoothGattServerManager.kt b/app/src/main/java/com/bitchat/android/mesh/BluetoothGattServerManager.kt index 038cd960..60f00a1b 100644 --- a/app/src/main/java/com/bitchat/android/mesh/BluetoothGattServerManager.kt +++ b/app/src/main/java/com/bitchat/android/mesh/BluetoothGattServerManager.kt @@ -282,15 +282,28 @@ class BluetoothGattServerManager( return } - if (BluetoothGattDescriptor.ENABLE_NOTIFICATION_VALUE.contentEquals(value)) { - connectionTracker.addSubscribedDevice(device) - - connectionScope.launch { - delay(100) - if (isActive) { // Check if still active - delegate?.onDeviceConnected(device) + when (GattSubscriptionPolicy.classify(value)) { + GattSubscriptionPolicy.Request.ENABLE -> { + // A repeat descriptor write is not a new connection. Without + // this gate a peer can re-enable notifications in a loop and + // drive both the duplicate-notify fan-out and the announce + // broadcast onDeviceConnected triggers, unauthenticated. + if (connectionTracker.addSubscribedDevice(device)) { + connectionScope.launch { + delay(100) + if (isActive) { // Check if still active + delegate?.onDeviceConnected(device) + } + } } } + + GattSubscriptionPolicy.Request.DISABLE -> + // The client asked to stop receiving the broadcast feed; + // keeping it subscribed until disconnect ignores that. + connectionTracker.removeSubscribedDevice(device) + + GattSubscriptionPolicy.Request.UNRECOGNIZED -> Unit } if (responseNeeded) { diff --git a/app/src/main/java/com/bitchat/android/mesh/GattSubscriptionPolicy.kt b/app/src/main/java/com/bitchat/android/mesh/GattSubscriptionPolicy.kt new file mode 100644 index 00000000..8254cf26 --- /dev/null +++ b/app/src/main/java/com/bitchat/android/mesh/GattSubscriptionPolicy.kt @@ -0,0 +1,34 @@ +package com.bitchat.android.mesh + +import android.bluetooth.BluetoothGattDescriptor + +/** + * Decides what a Client Characteristic Configuration Descriptor write means for + * our broadcast-notification subscription list. + * + * A CCCD write is the one GATT operation a peer can repeat freely on an open + * connection: it needs no authentication, no Noise session and no announcement, + * and the Bluetooth spec places no limit on how often a client may rewrite the + * descriptor. Every effect the server attaches to that write therefore has to be + * idempotent, or a peer gets to drive it at whatever rate it likes. + */ +internal object GattSubscriptionPolicy { + + enum class Request { + /** Client asked to receive broadcast notifications. */ + ENABLE, + + /** Client asked to stop receiving them. Honouring this is spec-required. */ + DISABLE, + + /** Any other descriptor value, including indications we do not use. */ + UNRECOGNIZED + } + + fun classify(value: ByteArray?): Request = when { + value == null -> Request.UNRECOGNIZED + BluetoothGattDescriptor.ENABLE_NOTIFICATION_VALUE.contentEquals(value) -> Request.ENABLE + BluetoothGattDescriptor.DISABLE_NOTIFICATION_VALUE.contentEquals(value) -> Request.DISABLE + else -> Request.UNRECOGNIZED + } +} diff --git a/app/src/test/kotlin/com/bitchat/android/mesh/GattBroadcastSubscriptionTest.kt b/app/src/test/kotlin/com/bitchat/android/mesh/GattBroadcastSubscriptionTest.kt new file mode 100644 index 00000000..c836391a --- /dev/null +++ b/app/src/test/kotlin/com/bitchat/android/mesh/GattBroadcastSubscriptionTest.kt @@ -0,0 +1,129 @@ +package com.bitchat.android.mesh + +import android.bluetooth.BluetoothDevice +import android.bluetooth.BluetoothGattDescriptor +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever +import org.robolectric.RobolectricTestRunner + +/** + * A CCCD write is the one GATT operation an unauthenticated peer can repeat at + * will on an open connection. Everything the server hangs off it — the broadcast + * fan-out list and the peer-connect side effects — therefore has to be + * idempotent, and an explicit unsubscribe has to be honoured. + */ +@RunWith(RobolectricTestRunner::class) +class GattBroadcastSubscriptionTest { + private val scope = CoroutineScope(Dispatchers.Unconfined + SupervisorJob()) + private val tracker = BluetoothConnectionTracker(scope, mock()) + + @After + fun tearDown() { + scope.cancel() + } + + private fun device(address: String): BluetoothDevice = mock().also { + whenever(it.address).thenReturn(address) + } + + @Test + fun `enabling notifications is recognized`() { + assertEquals( + GattSubscriptionPolicy.Request.ENABLE, + GattSubscriptionPolicy.classify(BluetoothGattDescriptor.ENABLE_NOTIFICATION_VALUE) + ) + } + + @Test + fun `disabling notifications is recognized instead of being ignored`() { + assertEquals( + GattSubscriptionPolicy.Request.DISABLE, + GattSubscriptionPolicy.classify(BluetoothGattDescriptor.DISABLE_NOTIFICATION_VALUE) + ) + } + + @Test + fun `any other descriptor value changes nothing`() { + assertEquals( + GattSubscriptionPolicy.Request.UNRECOGNIZED, + GattSubscriptionPolicy.classify(BluetoothGattDescriptor.ENABLE_INDICATION_VALUE) + ) + assertEquals(GattSubscriptionPolicy.Request.UNRECOGNIZED, GattSubscriptionPolicy.classify(null)) + assertEquals(GattSubscriptionPolicy.Request.UNRECOGNIZED, GattSubscriptionPolicy.classify(ByteArray(0))) + } + + @Test + fun `repeated enables subscribe an address exactly once`() { + val peer = device("AA:BB:CC:DD:EE:01") + + assertTrue("first enable must be a new subscription", tracker.addSubscribedDevice(peer)) + repeat(64) { + assertFalse("a repeat enable is not a new subscription", tracker.addSubscribedDevice(peer)) + } + + assertEquals(1, tracker.getSubscribedDevices().count { it.address == peer.address }) + } + + @Test + fun `a second framework instance for the same address does not add a second entry`() { + // The stack hands out distinct BluetoothDevice objects for one address, + // so identity comparison would let the fan-out list grow anyway. + assertTrue(tracker.addSubscribedDevice(device("AA:BB:CC:DD:EE:02"))) + assertFalse(tracker.addSubscribedDevice(device("AA:BB:CC:DD:EE:02"))) + + assertEquals(1, tracker.getSubscribedDevices().size) + } + + @Test + fun `unsubscribing removes the address whichever instance carries it`() { + assertTrue(tracker.addSubscribedDevice(device("AA:BB:CC:DD:EE:03"))) + + tracker.removeSubscribedDevice(device("AA:BB:CC:DD:EE:03")) + + assertTrue(tracker.getSubscribedDevices().isEmpty()) + assertFalse(tracker.isSubscribed("AA:BB:CC:DD:EE:03")) + } + + @Test + fun `unsubscribing one peer leaves the others receiving broadcasts`() { + val first = device("AA:BB:CC:DD:EE:04") + val second = device("AA:BB:CC:DD:EE:05") + tracker.addSubscribedDevice(first) + tracker.addSubscribedDevice(second) + + tracker.removeSubscribedDevice(first) + + assertEquals(listOf(second.address), tracker.getSubscribedDevices().map { it.address }) + } + + @Test + fun `resubscribing after an unsubscribe counts as new again`() { + val peer = device("AA:BB:CC:DD:EE:06") + + assertTrue(tracker.addSubscribedDevice(peer)) + tracker.removeSubscribedDevice(peer) + + assertTrue(tracker.addSubscribedDevice(peer)) + assertEquals(1, tracker.getSubscribedDevices().size) + } + + @Test + fun `disconnect cleanup still drops the subscription`() { + val peer = device("AA:BB:CC:DD:EE:07") + tracker.addSubscribedDevice(peer) + + tracker.cleanupDeviceConnection(peer.address) + + assertFalse(tracker.isSubscribed(peer.address)) + } +} diff --git a/docs/device_manager.md b/docs/device_manager.md index c42cd2c5..c19f9f35 100644 --- a/docs/device_manager.md +++ b/docs/device_manager.md @@ -55,6 +55,14 @@ Timers: - On connection setup complete (descriptor enable) and also after initial connect, start monitoring via `onConnectionEstablished(addr)`. - On packet write, call `deviceMonitor.onAnyPacketReceived(addr)`. - On disconnect, call `deviceMonitor.onDeviceDisconnected(addr, status)`. +- Broadcast-notification subscription is keyed by device address and is + idempotent. A CCCD write needs no authentication and the Bluetooth spec puts + no limit on how often a client may repeat it, so the server must treat a + repeat enable as a no-op: exactly one entry per address in the notification + fan-out list, and peer-connect side effects (including the announce broadcast + `onDeviceConnected` triggers) run only on the transition into the subscribed + state. A CCCD write of `DISABLE_NOTIFICATION_VALUE` unsubscribes the address; + it is not deferred to disconnect. 4) ANNOUNCE Binding - File: `BluetoothMeshService.kt` (in the ANNOUNCE handler where we first map device → peer)