diff --git a/android/app/src/main/java/com/eddy/atomrelay/UsbKeyReceiver.kt b/android/app/src/main/java/com/eddy/atomrelay/UsbKeyReceiver.kt index 7d1fb1c..6b58640 100644 --- a/android/app/src/main/java/com/eddy/atomrelay/UsbKeyReceiver.kt +++ b/android/app/src/main/java/com/eddy/atomrelay/UsbKeyReceiver.kt @@ -213,7 +213,7 @@ class UsbKeyReceiver : BroadcastReceiver() { val upperKey = hexKey.uppercase() val prefs = context.getSharedPreferences(SettingsActivity.PREFS_NAME, Context.MODE_PRIVATE) prefs.edit().putString(SettingsActivity.PREF_KEY, upperKey).apply() - Log.d(TAG, "HMAC key saved: $upperKey") + Log.d(TAG, "HMAC key saved (${upperKey.length} chars)") handler.post { Toast.makeText(context, "Beveiligingssleutel gelezen", Toast.LENGTH_LONG).show() } @@ -226,7 +226,7 @@ class UsbKeyReceiver : BroadcastReceiver() { } else if (noKey) { Log.d(TAG, "ESP has no key — generating and provisioning") val newKey = generateRandomHexKey() - Log.d(TAG, "Generated key: $newKey") + Log.d(TAG, "Generated key (${newKey.length} chars)") val setKeyCmd = "SET_KEY:$newKey\n".toByteArray(StandardCharsets.US_ASCII) val written = connection.bulkTransfer(epOut, setKeyCmd, setKeyCmd.size, 1000) diff --git a/codereview.md b/codereview.md new file mode 100644 index 0000000..e861f20 --- /dev/null +++ b/codereview.md @@ -0,0 +1,391 @@ +# Code Review — Sylvia Touch Relay Android App + +**Datum:** 26 juli 2026 +**Reviewer:** opencode (GLM-5.1) +**Scope:** Volledige Android app (`android/`) + +--- + +## Samenvatting + +De Android app voor de Sylvia Touch Relay is een goed functionerende BLE-client met HMAC-authenticatie, USB key provisioning en firmware flashing. De codebase is compact (~1300 regels Kotlin) en doet wat het moet doen. Er zijn echter enkele significante verbeterpunten rondom beveiliging, resource management, error handling en code organisatie. + +**Beoordeling:** Functioneel en bruikbaar, met aandachtspunten voor productie-kwaliteit. + +--- + +## Bevindingen per categorie + +### 1. Beveiliging + +#### 1.1 HMAC Key opslag in SharedPreferences (BELANGRIJK) + +**Bestand:** `SettingsActivity.kt:162`, `UsbKeyReceiver.kt:215,252` + +De HMAC key wordt opgeslagen in plain-text SharedPreferences. Dit is het primaire beveiligingsmechanisme van de app en zou met `EncryptedSharedPreferences` (Jetpack Security) beschermd moeten worden. Op een geroot device is de key nu triviaal uit te lezen via de prefs XML file. + +**Aanbeveling:** Gebruik `androidx.security:security-crypto` met `EncryptedSharedPreferences`: +```kotlin +val masterKey = MasterKey.Builder(context) + .setKeyScheme(MasterKey.KeyScheme.AES256_GCM) + .build() +val prefs = EncryptedSharedPreferences.create( + context, PREFS_NAME, masterKey, + EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, + EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM +) +``` + +#### 1.2 Sequence counter niet gepersisteerd (MEDIUM) + +**Bestand:** `MainActivity.kt:53` + +```kotlin +private var seq = 1 +``` + +De sequence counter reset naar 1 bij elke app-herstart. Na herstart kan de app een sequence sturen die lager is dan de laatste sequence die de ESP heeft ontvangen. De ESP's `lastSeq` reset ook bij reconnect, dus dit is alleen een probleem als de app herstart zonder BLE disconnect. In de praktijk valt dit mee omdat de ESP bij elke nieuwe BLE-verbinding de `lastSeq` reset. Maar persistentie van `seq` zou de replay-bescherming robuuster maken. + +**Aanbeveling:** Overweeg de sequence counter in SharedPreferences te persisten, óf документeer dat de ESP-side reset het dekt. + +#### 1.3 Log-aanvallen lekken key-materiaal (BELANGRIJK) + +**Bestand:** `UsbKeyReceiver.kt:216,229` + +```kotlin +Log.d(TAG, "HMAC key saved: $upperKey") +Log.d(TAG, "Generated key: $newKey") +``` + +De volledige HMAC key wordt naar logcat gelogd op DEBUG-niveau. Dit is een beveiligingslek: elke app met `READ_LOGS` of ADB-toegang kan deze key uitlezen. + +**Aanbeveling:** Verwijder deze log statements of vervang door: `Log.d(TAG, "HMAC key saved (${upperKey.length} chars)")`. + +--- + +### 2. Resource Management & Memory Leaks + +#### 2.1 BLE scan callback niet afgehandeld bij geen permissies (MEDIUM) + +**Bestand:** `MainActivity.kt:227-230` + +```kotlin +if (!hasPermissions()) { + requestPermissions() + return +} +``` + +Als permissies geweigerd worden en de gebruiker keert terug naar de app, wordt `startScan()` niet opnieuw aangeroepen. De app blijft hangen in "Geen permissies" zonder feedback. + +**Aanbeveling:** Bij geweigerde permissies in `onRequestPermissionsResult`, toon een uitleg of stuur de gebruiker naar app-instellingen. + +#### 2.2 Meerdere geneste Handler.postDelayed zonder cancel (MEDIUM) + +**Bestand:** `MainActivity.kt:89,147,157` + +Er zijn meerdere `handler.postDelayed` calls die BLE scans starten. Als `onResume()` meerdere keren wordt aangeroepen (bijv. bij snelle activity-transities), kunnen er meerdere scan-timers lopen. De `isScanning` guard helpt, maar dequeued niet de overtollige callbacks. + +**Aanbeveling:** Gebruik een specifieke `Runnable` in plaats van lambda's zodat deze met `handler.removeCallbacks(runnable)` afgebroken kunnen worden, óf gebruik een flag die ook pending scans voorkomt. + +#### 2.3 SettingsActivity receivers niet veilig geregistreerd op API 14+ (LAAG) + +**Bestand:** `SettingsActivity.kt:115-117` + +De `RECEIVER_NOT_EXPORTED` flag wordt gebruikt voor `usbPermissionReceiver` met een custom action `ACTION_USB_PERMISSION`. Dit is correct voor custom actions. Echter, de `usbAttachedReceiver` en `usbDetachedReceiver` zouden met `RECEIVER_NOT_EXPORTED` problemen kunnen geven omdat systeem-broadcasts (zoals `ACTION_USB_DEVICE_ATTACHED`) exported moeten zijn om ontvangen te worden. + +**Aanbeveling:** Gebruik `RECEIVER_EXPORTED` voor systeem-broadcasts (`ACTION_USB_DEVICE_ATTACHED`/`DETACHED`) en `RECEIVER_NOT_EXPORTED` voor de custom `ACTION_USB_PERMISSION`. + +#### 2.4 ProgressDialog is deprecated (MEDIUM) + +**Bestand:** `SettingsActivity.kt:153,204,258,272` + +`ProgressDialog` is deprecated sinds API level 26. Het werkt nog, maar wordt niet meer onderhouden en kan UI inconsistency veroorzaken op nieuwe Android versies. + +**Aanbeveling:** Vervang door `AlertDialog` met een `ProgressBar` of `MaterialAlertDialogBuilder` met een custom layout. + +--- + +### 3. Concurrency & Threading + +#### 3.1 Background threads zonder lifecycle-awareness (BELANGRIJK) + +**Bestand:** `SettingsActivity.kt:155-178` + +```kotlin +Thread { + val result = readKeyFromEsp(usbManager, device) + if (isFinishing || isDestroyed) return@Thread + handler.post { ... } +}.start() +``` + +De `isFinishing || isDestroyed` check is goed, maar komt ná een potentiële 15-seconden USB operatie. Als de Activity vernietigd wordt tijdens de USB-leesoperatie, draait de thread nutteloos door en wordt er nog USB I/O gedaan op een vernietigde context. + +**Aanbeveling:** Gebruik coroutines met `lifecycleScope` voor betere cancellation: +```kotlin +lifecycleScope.launch(Dispatchers.IO) { + val result = readKeyFromEsp(usbManager, device) + withContext(Dispatchers.Main) { ... } +} +``` + +#### 3.2 EspFlasher thread kan niet geannuleerd worden (MEDIUM) + +**Bestand:** `EspFlasher.kt:29-37` + +De flasher draait in een raw `Thread` die niet annuleerbaar is. Als de gebruiker de Activity verlaat tijdens het flashen (wat de `setCancelable(false)` probeert te voorkomen), draait de thread door. + +**Aanbeveling:** Overweeg een cancelable mechanisme of gebruik coroutines. + +#### 3.3 BLE writes zonder acknowledgment fallback (LAAG) + +**Bestand:** `MainActivity.kt:277-289` + +`WRITE_TYPE_NO_RESPONSE` betekent dat er geen bevestiging komt van de ESP. Als de packet verloren gaat, heeft de app geen retry mechanisme. De knop-toestand wijzigt wel ("ON"), maar het relais schakelt niet. + +**Aanbeveling:** Voor productie: overweeg een bevestigingsmechanisme via BLE notifications van de ESP, om de UI-state te koppelen aan de werkelijke relay-state. + +--- + +### 4. Code Organisatie & Onderhoudbaarheid + +#### 4.1 UsbKeyReceiver dupliceert SettingsActivity logica (BELANGRIJK) + +**Bestand:** `UsbKeyReceiver.kt` vs `SettingsActivity.kt:314-395` + +`UsbKeyReceiver` bevat een volledige kopie van de CDC key-exchange logica die ook in `SettingsActivity.readKeyFromEsp()` staat. Beide implementeren: +- CDC interface finding +- CDC control setup (line coding, DTR/RTS) +- KEY? command + response parsing +- SET_KEY command + OK verification +- Random key generation + +Daarnaast heeft `UsbKeyReceiver` zijn eigen kopie van `findCdcDataInterface`, `findBulkEndpoint`, en `findCdcControlInterface` — deze bestaan ook al in `UsbUtils`. + +**Aanbeveling:** Extracteer de key-exchange logica naar een gedeelde class (bijv. `UsbKeyExchange`) die door beide callers gebruikt wordt. Gebruik `UsbUtils` consequent. + +#### 4.2 UsbKeyReceiver niet geregistreerd in AndroidManifest (MEDIUM) + +**Bestand:** `UsbKeyReceiver.kt` — niet in `AndroidManifest.xml` + +`UsbKeyReceiver` is defined als een `BroadcastReceiver` maar wordt nergens geregistreerd (noch statisch in AndroidManifest, noch dynamisch in een Activity). Het is dode code. + +**Aanbeveling:** Of registreer de receiver (dynamisch in `MainActivity` of statisch in Manifest met `intent-filter` voor `ACTION_USB_DEVICE_ATTACHED`), of verwijder de class als de functionaliteit via `SettingsActivity` voldoende is. + +#### 4.3 Magic numbers en hardcoded waarden (LAAG) + +**Bestand:** `MainActivity.kt:43-44`, `SettingsActivity.kt:34-37` + +Constants zoals `SCAN_TIMEOUT_MS`, `ESP_VENDOR_ID`, `KEY_TIMEOUT_MS` zijn gedefinieerd — dit is goed. Maar er zijn ook ongedefinieerde magic numbers: +- `MainActivity.kt:250` — `SCAN_TIMEOUT_MS` (gedefinieerd, goed) +- `SettingsActivity.kt:334` — retry count `3` als literal +- `SettingsActivity.kt:336,347,373` — `Thread.sleep(500)` hardcoded +- `UsbSerialTransport.kt:16,22` — timeout `5000` hardcoded als magic number + +**Aanbeveling:** Definieer deze als constants in de companion objects. + +#### 4.4 getString() niet gebruikt voor user-facing tekst (LAAG) + +**Bestand:** meerdere — bijv. `MainActivity.kt:155,234,245`, `SettingsActivity.kt:222,237` + +User-facing strings zijn hardcoded in Kotlin: +```kotlin +statusText.text = "Geen HMAC sleutel - open Instellingen" +statusText.text = "Verbinden..." +``` + +`strings.xml` bevat slechts 5 entries die grotendeels ongebruikt zijn. + +**Aanbeveling:** Verplaats alle user-facing strings naar `strings.xml` en gebruik `getString(R.string.xxx)` voor lokalisatie-ondersteuning en consistentie. + +#### 4.5 Geen proguard rules voor de flasher library (LAAG) + +**Bestand:** `app/build.gradle:26` + +`minifyEnabled false` voor release. Als men in de toekomst minify aanzet, kan de `esp32-flash-lib` obfuscation-breuk lijden zonder proguard rules. + +**Aanbeveling:** Voeg proguard rules toe voor `org.dkaukov.esp32.*` wanneer minify geactiveerd wordt. + +--- + +### 5. Error Handling + +#### 5.1 Geen handling voor BLE write failure (BELANGRIJK) + +**Bestand:** `MainActivity.kt:284-287` + +```kotlin +char.value = packet +char.writeType = BluetoothGattCharacteristic.WRITE_TYPE_NO_RESPONSE +gatt.writeCharacteristic(char) +seq++ +``` + +Op Android API < 13 retourneert `writeCharacteristic()` een boolean die succes/falen aangaf. Op API 13+ is het callback-based. Er wordt geen `onCharacteristicWrite` callback geïmplementeerd in `gattCallback`. Als de write faalt (bijv. characteristic niet beschikbaar, buffer vol), wordt de sequence counter toch opgehoogd, wat leidt tot desync. + +**Aanbeveling:** Implementeer `onCharacteristicWrite` callback en verhoog `seq` pas na bevestiging. + +#### 5.2 GATT status codes niet gecontroleerd (MEDIUM) + +**Bestand:** `MainActivity.kt:73,94,100` + +```kotlin +override fun onConnectionStateChange(gatt: BluetoothGatt, status: Int, newState: Int) { +``` + +De `status` parameter wordt genegeerd. Wanneer `status != GATT_SUCCESS`, betekent dit dat de verbinding onverwacht verbroken is (niet een normale disconnect). De app behandelt en behandelt alle disconnects hetzelfde (reconnect na 2s), maar logt niet wat de oorzaak was. + +**Aanbeveling:** Log de status code bij faalsituaties: +```kotlin +if (status != BluetoothGatt.GATT_SUCCESS) { + Log.w(TAG, "GATT error: $status, state: $newState") +} +``` + +#### 5.3 Broakkoppel exception swallowing (LAAG) + +**Bestand:** `SettingsActivity.kt:123-125`, `393` + +```kotlin +try { unregisterReceiver(usbAttachedReceiver) } catch (e: Exception) {} +try { connection.close() } catch (e: Exception) {} +``` + +Exceptions worden leeg gevangen. Dit kan bugs maskeren. + +**Aanbeveling:** Log minimaal op WARNING niveau: +```kotlin +try { unregisterReceiver(usbAttachedReceiver) } catch (e: Exception) { + Log.w(TAG, "Failed to unregister receiver", e) +} +``` + +--- + +### 6. UI & UX + +#### 6.1 Knop-state niet gesynchroniseerd met relay-state (MEDIUM) + +**Bestand:** `MainActivity.kt:207-222` + +De knop tekst wijzigt naar "ON" bij `ACTION_DOWN` en "HOLD" bij `ACTION_UP`, ongeacht of het BLE-commando daadwerkelijk is aangekomen. Gezien het gebruik van `WRITE_TYPE_NO_RESPONSE` is er geen bevestiging. + +**Aanbeveling:** Geef visuele feedback die duiderscheid maakt tussen "commando verzonden" en "relais bevestigd" — bijv. een dimmed state of pending indicator. + +#### 6.2 Geen dark mode support (LAAG) + +**Bestand:** `AndroidManifest.xml:17` + +```xml +android:theme="@style/Theme.AppCompat.Light" +``` + +De app forceert Light theme, ook op devices in dark mode. + +**Aanbeveling:** Gebruik `Theme.AppCompat.DayNight` of `Theme.Material3.DayNight` voor automatisch dark mode support. + +#### 6.3 Geen visuele indicator voor scan timeout (LAAG) + +**Bestand:** `MainActivity.kt:264-265` + +Wanneer de scan timeout bereikt wordt: +```kotlin +if (scanTimedOut) { + statusText.text = "Device not found" +} +``` + +Er is geen retry-knop of automatische retry. De gebruiker moet de app herstarten. + +**Aanbeveling:** Voeg een "Opnieuw scannen" knop toe of retry automatisch na 5 seconden. + +--- + +### 7. Build & Dependencies + +#### 7.1 Verouderde AGP versie (MEDIUM) + +**Bestand:** `android/build.gradle:2` + +```groovy +id 'com.android.application' version '8.2.2' apply false +``` + +AGP 8.2.2 is van begin 2024. De nieuwste is 8.5+. Nieuwere versies bevatten bug fixes en betere Kotlin support. + +**Aanneveling:** Upgrade AGP naar 8.5+ (compatibel met Gradle 8.14). + +#### 7.2 compileSdk 34, niet 35 (LAAG) + +**Bestand:** `app/build.gradle:8,13` + +`compileSdk` en `targetSdk` zijn 34 (Android 14). Android 15 (API 35) is uitgebracht. + +**Aanbeveling:** Upgrade naar SDK 35 om de nieuwste API's en security fixes te gebruiken. + +#### 7.3 Kotlin 1.9.24, niet 2.x (LAAG) + +**Bestand:** `android/build.gradle:3` + +Kotlin 1.9.24 is ouder dan Kotlin 2.0+. Kotlin 2.0 biedt K2 compiler met betere prestaties. + +**Aanneveling:** Overweeg upgrade naar Kotlin 2.0+ (vereist AGP 8.5+). + +--- + +### 8. Firmware / App inconsistenties + +#### 8.1 Line coding byte-array verschilt (MEDIUM) + +**Bestand:** `SettingsActivity.kt:430-433` vs `UsbKeyReceiver.kt:164` + +```kotlin +// SettingsActivity: +byteArrayOf(0x00, 0xC2.toByte(), 0x01, 0x00, 0x00, 0x08, 0x08) +// UsbKeyReceiver: +byteArrayOf(0x00, 0xC2.toByte(), 0x01, 0x00, 0x00, 0x00, 0x08) +``` + +Het 6e byte verschilt: `0x08` (SettingsActivity) vs `0x00` (UsbKeyReceiver). De juiste CDC line coding is `115200 8N1`: +- Byte 0-3: baudrate little-endian (115200 = `0x00C20000` → `0x00, 0xC2, 0x01, 0x00`) +- Byte 4: stop bits (0 = 1 stop bit) +- Byte 5: parity (0 = none) +- Byte 6: data bits (8) + +`UsbKeyReceiver` heeft de juiste waarde (`0x00` voor parity). `SettingsActivity` heeft `0x08` — dit zegt parity=space. Het werkt waarschijnlijk omdat de ESP32-S3 CDC deze instellingen negeert, maar het is inconsistent. + +**Aanbeveling:** Fix `SettingsActivity.kt:432` naar `0x00` en extracteer naar een gedeelde functie. + +--- + +## Prioritering + +| Prioriteit | Aantal | Actie | +|---|---|---| +| **Belangrijk** | 5 | Na ontvangst oplossen | +| **MEDIUM** | 8 | Bij volgende release | +| **LAAG** | 8 | Optioneel / technische schuld | + +--- + +## Top 5 aanbevelingen (snelste impact) + +1. **Verwijder key logging** (`UsbKeyReceiver.kt:216,229`) — 2 regels, direct beveiligingswinst +2. **Schakel over naar EncryptedSharedPreferences** — beschermt de HMAC key op device +3. **Consolideer UsbKeyReceiver** — 318 regels dode code of duplicatie; verwijderen of integreren +4. **Fix line coding byte** (`SettingsActivity.kt:432`) — consistentie bug +5. **Implementeer `onCharacteristicWrite`** — voorkomt sequence desync + +--- + +## Conclusie + +De app is solide voor hobbyist-gebruik en bevat doordachte beveiligingsarchitectuur (HMAC, sequence counters, USB key provisioning). Voor productie-kwaliteit zijn de belangrijkste punten: + +- **Beveiliging:** Key-opslag versleutelen, logging van key-materiaal verwijderen +- **Code kwaliteit:** Duplicatie weghalen (UsbKeyReceiver), coroutines ipv raw Threads +- **Robustheid:** BLE write acknowledgements, GATT status logging +- **Modernisering:** EncryptedSharedPreferences, DayNight theme, AGP/Kotlin upgrade + +De firmware (`src/main.cpp`) is netjes geschreven en heeft geen aanbevelingen buiten scope van deze review. \ No newline at end of file