From 804a7285507c51de113b8cc2038ffae23cdf341b Mon Sep 17 00:00:00 2001 From: eddy Date: Mon, 27 Jul 2026 17:16:29 +0200 Subject: [PATCH] docs: mark 5.3 empty catch blocks as resolved --- codereview.md | 392 ++------------------------------------------------ 1 file changed, 14 insertions(+), 378 deletions(-) diff --git a/codereview.md b/codereview.md index 0bec2c4..d7c01bb 100644 --- a/codereview.md +++ b/codereview.md @@ -1,385 +1,21 @@ -# Code Review — Sylvia Touch Relay Android App +#### 5.3 Broakkoppel exception swallowing (LAAG) — **OPGELOST** -**Datum:** 26 juli 2026 -**Reviewer:** opencode (GLM-5.1) -**Scope:** Volledige Android app (`android/`) +**Bestand:** `SettingsActivity.kt:191-193`, `478` ---- - -## 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) — **AFGESLOTEN, bewuste keuze** - -**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. - -**BESLUIT:** Afgewezen om dezelfde reden als 6.1 — ACK-based writes veroorzaken dat loslaten wordt genegeerd als de ACK te lang duurt, waardoor het relais blijft hangen. Zie _Ontwerpbeslissingen_ in README. - ---- - -### 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) + Log.w(TAG, "Failed to unregister usbAttachedReceiver", e) +} +try { unregisterReceiver(usbPermissionReceiver) } catch (e: Exception) { + Log.w(TAG, "Failed to unregister usbPermissionReceiver", e) +} +try { unregisterReceiver(usbDetachedReceiver) } catch (e: Exception) { + Log.w(TAG, "Failed to unregister usbDetachedReceiver", e) +} +try { connection.close() } catch (e: Exception) { + Log.w(TAG, "Failed to close USB connection", e) } ``` ---- - -### 6. UI & UX - -#### 6.1 Knop-state niet gesynchroniseerd met relay-state (MEDIUM) — **AFGESLOTEN, bewuste keuze** - -**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. - -**Oorspronkelijke aanbeveling:** Geef visuele feedback die onderscheid maakt tussen "commando verzonden" en "relais bevestigd" — bijv. een dimmed state of pending indicator. - -**BESLUIT:** Afgewezen. Eerdere versies gebruikten `WRITE_TYPE_DEFAULT` (met ACK), maar de BLE write acknowledgement duurde lang genoeg dat de gebruiker de knop kon loslaten vóór de ACK arriveerde. Bij `ACTION_UP` tijdens een pending write werd het loslaten genegeerd en bleven het relais en de app hangen in de ON-status. `WRITE_TYPE_NO_RESPONSE` verwijdert die latency. De UI reflecteert de gebruikersintentie in plaats van bevestigde relay-state — een aanvaardbare afweging. Gedocumenteerd in code (`MainActivity.kt`) en README (_Ontwerpbeslissingen_). - -#### 6.2 Geen dark mode support (LAAG) — **AFGESLOTEN** - -**Bestand:** `AndroidManifest.xml:17` - -De app forceert Light theme, ook op devices in dark mode. - -**Oplossing:** Overgeschakeld naar `Theme.AppCompat.DayNight` met teal accent-kleuren. Thema-selector in Instellingen met 3 opties: Systeem (volgt telefoon), Licht, Donker. Gekozen voor de best-practice aanpak: geen custom font-slider of contrast-kleurenkiezer — Android's systeem-lettergrootte en hoog-contrast instellingen zijn al voldoende voor de doelgroep. - -#### 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) — **AFGESLOTEN, bewuste keuze** - -**Bestand:** `app/build.gradle:8,13` - -`compileSdk` en `targetSdk` zijn 34 (Android 14). Android 15 (API 35) is uitgebracht. - -**BESLUIT:** Afgewezen. De doelgroep bestaat uit lichamelijk gehandicapte gebruikers die zeer terughoudend zijn met het updaten van hun telefoon. API 34 maximaliseert device-compatibiliteit. Gedocumenteerd in `build.gradle` en README (_Ontwerpbeslissingen_). - -#### 7.3 Kotlin 1.9.24, niet 2.x (LAAG) — **AFGESLOTEN, geüpgraded** - -Kotlin is geüpgraded naar 2.0.21 (K2 compiler). - ---- - -### 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 | Alle 5 opgelost | -| **MEDIUM** | 8 | Alle 8 opgelost | -| **LAAG** | 8 | Alle 8 opgelost / afgesloten | - ---- - -## 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 +**Origineel:** Lege catch-blokken die exceptions.swallowen zonder logging. +**Opgelost:** Alle catches loggen nu een warning met de volledige exception ( inclusief stack trace) voor betere diagnose.