fix(security,race,ux): close 6 high-priority bugs (B1–B6)

Wave 1 — CRITICAL (token + PII leak): B1 (kanban t_afc1d26f): Auth token leaked to logcat via HttpLoggingInterceptor.Level.BODY combined with the authInterceptor that adds `Authorization: Bearer *** on every REST call. Enabled buildConfig=true in app/build.gradle.kts and gate Level.BODY on BuildConfig.DEBUG (Level.NONE in release). ApiClient.kt:44-56. B2 (kanban t_8884db16): Chat prompts + AI replies leaked to logcat via unguarded Log.d in HermesWsClient. Gated the 3 PII-critical sites (line 104 outgoing JSON which contains user `text`, line ~123 connecting URL whose query string carries the auth token, line ~181 incoming WS payload with AI reply tokens and tool output) with BuildConfig.DEBUG. Log.i/.w/.e left unguarded as intentional ops signals. Wave 2 — HIGH (races + thread-pattern): B3 (kanban t_33da8a97): switchSession fired SESSION_RESUME (WS) and loadSessionMessages (REST) in parallel. The REST success path did an unconditional `_uiState.update { it.copy(messages = chatMessages) }` that stomped any in-flight streaming tokens appended via WsEvent.MessageToken. ChatViewModel.loadSessionMessages now preserves messages with isStreaming=true by re-appending them to the freshly loaded history. Bonus side-fix discovered in same function: isLoading was never set to false on success or error branches — now explicitly cleared on all exit paths. B4 (kanban t_2b834d90): HermesWsClient.scheduleReconnect used a raw Thread{} with Thread.sleep + a non-atomic check-then-openSocket() (TOCTOU). Two reconnect threads spawned by a rapid onFailure/onClosed cycle could both pass the gate and both call openSocket(), the second overwriting `webSocket` without closing the first — orphaned socket listener leak. Flaky networks multiplied daemon thread count. Replaced with a single cancellable coroutine Job keyed to wsScope (Dispatchers.IO + SupervisorJob). disconnect() now cancels any pending reconnect so a delayed openSocket() can't fire after explicit close. Wave 3 — MEDIUM (UX gaps + persistence): B5 (kanban t_2322818d): LogsScreen rendered a blank black Box when state.logs was empty && !isLoading && errorMessage == null. Added an explicit empty-state branch rendering "No logs available" in onSurfaceVariant, centered. Mirrors sibling pattern in PairingScreen.kt:200-219 and WebhooksScreen.kt:171-190. B6 (kanban t_86e9be9b): Theme preference was in-memory only — lost on every cold start. Verification confirmed three gaps: (1) SettingsViewModel.loadSettings() didn't restore themePreference from storage; (2) SettingsViewModel.save() didn't persist it; (3) MainActivity called `HermesControlTheme {}` with no themePreference arg so even if storage had worked, the app wouldn't apply it at startup. Added getThemePreference/setThemePreference in AuthManager (EncryptedSharedPreferences round-trip, validate stored enum name with runCatching to handle forward-incompatible strings), wired SettingsViewModel init + save, and passed the stored preference to HermesControlTheme(themePreference = AuthManager.getThemePreference()) at startup. Verification: - ktlintCheck: BUILD SUCCESSFUL in 7s (ktlint app/src/**/*.kt — clean) - ./gradlew test: NOT RUN on this host (no Android SDK at $ANDROID_HOME); deferred to the CI workflow at .github/workflows/android.yml. Same limitation applies to assembleRelease. Symbol-resolution reasoning: app/build.gradle.kts:10 declares namespace=com.m57.hermescontrol so AGP auto-generates the BuildConfig class at package com.m57.hermescontrol — matches the new imports in ApiClient.kt and HermesWsClient.kt. Kanban tracking (native Hermes kanban, board slug "hermes-mobile"): B1 t_afc1d26f done - Auth token leak via Level.BODY B2 t_8884db16 done - Chat PII via unguarded Log.d B3 t_33da8a97 done - switchSession race stomps streaming tokens B4 t_2b834d90 done - WS reconnect TOCTOU + unbounded Thread{} B5 t_2322818d done - LogsScreen missing empty-state B6 t_86e9be9b done - Theme preference not persisted B7 t_74f3b383 ready - (deferred) NavigationController.goBack underflow B8 t_39a149f0 ready - (deferred) KanbanBoardsResponse.current nullable B9 t_4b1465cf ready - (deferred) E2eIntegrationTest base64 no-op via Mockk B10 t_81ef4203 ready - (deferred) ChannelsScreen form state on scroll B11 t_3325498b ready - (deferred) TEST_READY/TEST_INFRA stale (28 vs 123) B12 t_674007f5 ready - (deferred) Hardcoded http:// (LAN assumption) B13 t_179009d5 ready - (deferred) SystemViewModel silent doctor catch Total: 8 files changed, +144 insertions, -26 deletions. Co-Authored-By: Ronia <ronia@hermes>
2026-06-18 02:17:11 +03:00
1 parent bd439ba674
commit a0b4ce8dc0
@@ -31,7 +31,7 @@ android {
    buildFeatures {
      compose = true
      aidl = false
-      buildConfig = false
+      buildConfig = true
      shaders = false
    }

@@ -19,7 +19,11 @@ class MainActivity : ComponentActivity() {

        enableEdgeToEdge()
        setContent {
-            HermesControlTheme {
+            // B6 (Jun 18 2026, kanban t_86e9be9b): read the stored theme
+            // preference at startup so HermesControlTheme actually applies
+            // the user's SYSTEM/LIGHT/DARK choice rather than defaulting to
+            // SYSTEM on every cold start.
+            HermesControlTheme(themePreference = AuthManager.getThemePreference()) {
                Surface(
                    modifier = Modifier.fillMaxSize(),
                    color = MaterialTheme.colorScheme.background,
@@ -4,6 +4,7 @@ import android.content.Context
 import android.content.SharedPreferences
 import androidx.security.crypto.EncryptedSharedPreferences
 import androidx.security.crypto.MasterKey
+import com.m57.hermescontrol.theme.ThemePreference

 /**
 * Singleton that manages encrypted storage of the Hermes dashboard token
@@ -18,6 +19,7 @@ object AuthManager {
    private const val KEY_HOST = "host"
    private const val KEY_PORT = "port"
    private const val KEY_AUTO_RECONNECT = "auto_reconnect"
+    private const val KEY_THEME_PREFERENCE = "theme_preference"

    private const val DEFAULT_HOST = "127.0.0.1"
    private const val DEFAULT_PORT = 9119
@@ -88,6 +90,25 @@ object AuthManager {
        requirePrefs().edit().putBoolean(KEY_AUTO_RECONNECT, enabled).apply()
    }

+    // ── Theme preference ──────────────────────────────────────────────────
+    // B6 (Jun 18 2026, kanban t_86e9be9b): previously, SettingsScreen.kt let
+    // the user pick SYSTEM/LIGHT/DARK in a SingleChoiceSegmentedButtonRow
+    // (SettingsScreen.kt:213-238) and SettingsViewModel.onThemeChange
+    // (SettingsViewModel.kt:62-64) updated in-memory state — but
+    // SettingsViewModel.loadSettings() did NOT read the value back and
+    // SettingsViewModel.save() did NOT persist it. As a result, every cold
+    // start reset the theme to SYSTEM. Fix: round-trip via EncryptedSharedPreferences
+    // (the same store already used by token / host / port / auto_reconnect).
+
+    fun getThemePreference(): ThemePreference =
+        requirePrefs().getString(KEY_THEME_PREFERENCE, ThemePreference.SYSTEM.name)?.let { name ->
+            runCatching { ThemePreference.valueOf(name) }.getOrNull()
+        } ?: ThemePreference.SYSTEM
+
+    fun setThemePreference(theme: ThemePreference) {
+        requirePrefs().edit().putString(KEY_THEME_PREFERENCE, theme.name).apply()
+    }
+
    /** Convenience: build the base URL from current host + port. */
    fun baseUrl(): String = "http://${getHost()}:${getPort()}/"

@@ -1,5 +1,6 @@
 package com.m57.hermescontrol.data.remote

+import com.m57.hermescontrol.BuildConfig
 import com.m57.hermescontrol.data.local.AuthManager
 import okhttp3.Interceptor
 import okhttp3.OkHttpClient
@@ -41,9 +42,18 @@ object ApiClient {
    // ── Internal ─────────────────────────────────────────────────────────

    private fun buildService(): HermesApiService {
+        // B1 (Jun 18 2026, kanban t_afc1d26f): Level.BODY prints every request —
+        // including the Authorization: Bearer header from authInterceptor below —
+        // to logcat in plaintext. Gate on BuildConfig.DEBUG so release builds
+        // are silent; debug builds behave identically to before.
        val logging =
            HttpLoggingInterceptor().apply {
-                level = HttpLoggingInterceptor.Level.BODY
+                level =
+                    if (BuildConfig.DEBUG) {
+                        HttpLoggingInterceptor.Level.BODY
+                    } else {
+                        HttpLoggingInterceptor.Level.NONE
+                    }
            }

        val authInterceptor =
@@ -2,10 +2,17 @@ package com.m57.hermescontrol.data.ws

 import android.util.Log
 import com.google.gson.Gson
+import com.m57.hermescontrol.BuildConfig
 import com.m57.hermescontrol.data.local.AuthManager
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.Job
+import kotlinx.coroutines.SupervisorJob
+import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.MutableSharedFlow
 import kotlinx.coroutines.flow.SharedFlow
 import kotlinx.coroutines.flow.asSharedFlow
+import kotlinx.coroutines.launch
 import okhttp3.OkHttpClient
 import okhttp3.Request
 import okhttp3.Response
@@ -44,6 +51,15 @@ object HermesWsClient {
    @Volatile
    private var currentBackoff = INITIAL_BACKOFF_MS

+    // B4 (Jun 18 2026, kanban t_2b834d90): dedicated IO coroutine scope for
+    // the new single-Job reconnect scheduler below. SupervisorJob keeps one
+    // child failure from cancelling the scope; the scope itself lives as
+    // long as this singleton (object) exists.
+    private val wsScope = CoroutineScope(Dispatchers.IO + SupervisorJob())
+
+    @Volatile
+    private var reconnectJob: Job? = null
+
    private val okHttpClient =
        OkHttpClient
            .Builder()
@@ -83,6 +99,11 @@ object HermesWsClient {
    /** Cleanly close the WebSocket and stop auto-reconnect. */
    fun disconnect() {
        intentionalClose.set(true)
+        // B4 (Jun 18 2026, kanban t_2b834d90): also cancel any pending
+        // reconnect coroutine so a scheduled openSocket() can't fire after
+        // the user explicitly disconnected.
+        reconnectJob?.cancel()
+        reconnectJob = null
        webSocket?.close(1000, "Client closed")
        webSocket = null
        connected.set(false)
@@ -101,7 +122,10 @@ object HermesWsClient {
        val id = requestId.incrementAndGet().toString()
        val request = JsonRpcRequest(id = id, method = method, params = params)
        val json = gson.toJson(request)
-        Log.d(TAG, "→ $json")
+        // B2 (Jun 18 2026, kanban t_8884db16): outgoing JSON contains user
+        // prompts (PromptSubmit params include the `text` field) — never
+        // stream to logcat in release builds.
+        if (BuildConfig.DEBUG) Log.d(TAG, "→ $json")
        webSocket?.send(json) ?: Log.w(TAG, "send() called while disconnected")
        return id
    }
@@ -120,7 +144,9 @@ object HermesWsClient {

    private fun openSocket() {
        val url = AuthManager.wsUrl()
-        Log.d(TAG, "Connecting to $url")
+        // B2 (Jun 18 2026, kanban t_8884db16): url contains the auth token
+        // as a query param — never stream to logcat in release builds.
+        if (BuildConfig.DEBUG) Log.d(TAG, "Connecting to $url")

        val request = Request.Builder().url(url).build()
        webSocket = okHttpClient.newWebSocket(request, WsListenerImpl())
@@ -129,7 +155,7 @@ object HermesWsClient {
    private fun scheduleReconnect() {
        if (intentionalClose.get()) return
        if (!AuthManager.isAutoReconnect()) {
-            Log.d(TAG, "Auto-reconnect disabled")
+            if (BuildConfig.DEBUG) Log.d(TAG, "Auto-reconnect disabled")
            return
        }
        val delay = currentBackoff
@@ -137,23 +163,32 @@ object HermesWsClient {
            (currentBackoff * BACKOFF_MULTIPLIER)
                .toLong()
                .coerceAtMost(MAX_BACKOFF_MS)
-        Log.d(TAG, "Reconnecting in ${delay}ms …")
+        if (BuildConfig.DEBUG) Log.d(TAG, "Reconnecting in ${delay}ms …")

-        // Simple thread-based scheduling; could be replaced with coroutine delay.
-        Thread {
-            try {
-                Thread.sleep(delay)
-            } catch (_: InterruptedException) {
-                return@Thread
+        // B4 (Jun 18 2026, kanban t_2b834d90): replaced raw Thread{ sleep;
+        // check; openSocket() } with a single cancellable coroutine Job.
+        //
+        // Old code had two failure modes:
+        //   1. TOCTOU: line 149 `if (!intentionalClose && !connected)` was
+        //      not atomic with line 150 `openSocket()`. Two reconnect threads
+        //      spawned from a rapid onFailure/onClosed cycle could both pass
+        //      the gate and both call openSocket() — the second overwrote
+        //      `webSocket` without closing the first, leaking an orphaned
+        //      listener.
+        //   2. Unbounded thread spawn: a flaky network multiplied the
+        //      daemon-thread count. Backoff grew but old threads weren't
+        //      cancelled.
+        //
+        // New behavior: cancel any in-flight reconnect before scheduling the
+        // next one — only one reconnect coroutine exists at any time.
+        reconnectJob?.cancel()
+        reconnectJob =
+            wsScope.launch {
+                delay(delay)
+                if (!intentionalClose.get() && !connected.get()) {
+                    openSocket()
+                }
            }
-            if (!intentionalClose.get() && !connected.get()) {
-                openSocket()
-            }
-        }.apply {
-            isDaemon = true
-            name = "hermes-ws-reconnect"
-            start()
-        }
    }

    private fun emit(event: WsEvent) {
@@ -178,7 +213,10 @@ object HermesWsClient {
            ws: WebSocket,
            text: String,
        ) {
-            Log.d(TAG, "← $text")
+            // B2 (Jun 18 2026, kanban t_8884db16): incoming WS text contains
+            // AI reply tokens and tool output — never stream to logcat in
+            // release builds.
+            if (BuildConfig.DEBUG) Log.d(TAG, "← $text")
            try {
                val rpc = gson.fromJson(text, JsonRpcResponse::class.java)
                val event = EventParser.parse(rpc, text)
@@ -526,12 +526,37 @@ class ChatViewModel : ViewModel() {
                                isStreaming = false,
                            )
                        }
-                    _uiState.update { it.copy(messages = chatMessages) }
+                    // B3 (Jun 18 2026, kanban t_33da8a97): switchSession fires
+                    // SESSION_RESUME (WS) and loadSessionMessages (REST) in
+                    // parallel. While this REST call is in flight, WsEvent
+                    // .MessageToken events append streaming messages to
+                    // `state.messages` (ChatViewModel.kt:99-125). Without
+                    // preservation, the unconditional `messages = chatMessages`
+                    // below stomps the just-streamed tokens. Preserve the
+                    // in-flight tail by appending any messages marked
+                    // isStreaming=true to the freshly-loaded history.
+                    _uiState.update { state ->
+                        val streamingTail = state.messages.filter { it.isStreaming }
+                        state.copy(
+                            messages = chatMessages + streamingTail,
+                            isLoading = false,
+                        )
+                    }
                } else {
-                    _uiState.update { it.copy(errorMessage = "Failed to load messages: HTTP ${response.code()}") }
+                    _uiState.update {
+                        it.copy(
+                            isLoading = false,
+                            errorMessage = "Failed to load messages: HTTP ${response.code()}",
+                        )
+                    }
                }
            } catch (e: Exception) {
-                _uiState.update { it.copy(errorMessage = "Failed to load messages: ${e.message}") }
+                _uiState.update {
+                    it.copy(
+                        isLoading = false,
+                        errorMessage = "Failed to load messages: ${e.message}",
+                    )
+                }
            }
        }
    }
@@ -107,6 +107,19 @@ fun LogsScreen(
                            .align(Alignment.Center)
                            .padding(16.dp),
                )
+            } else if (state.logs.isEmpty()) {
+                // B5 (Jun 18 2026, kanban t_2322818d): explicit empty-state UX.
+                // Sibling screens (PairingScreen.kt, WebhooksScreen.kt) did this;
+                // LogsScreen didn't — when logs were empty and not loading, the
+                // black Box rendered with NO content visible to the user.
+                Text(
+                    text = "No logs available",
+                    color = MaterialTheme.colorScheme.onSurfaceVariant,
+                    modifier =
+                        Modifier
+                            .align(Alignment.Center)
+                            .padding(16.dp),
+                )
            } else {
                LazyColumn(
                    state = listState,
@@ -39,6 +39,10 @@ class SettingsViewModel : ViewModel() {
                port = AuthManager.getPort().toString(),
                token = AuthManager.getToken() ?: "",
                autoReconnect = AuthManager.isAutoReconnect(),
+                // B6 (Jun 18 2026, kanban t_86e9be9b): restore user's theme
+                // choice from persistent storage on init. Previously this slot
+                // was missing — always defaulted to ThemePreference.SYSTEM.
+                themePreference = AuthManager.getThemePreference(),
            )
        }
    }
@@ -71,6 +75,9 @@ class SettingsViewModel : ViewModel() {
        AuthManager.setPort(port)
        AuthManager.setToken(state.token)
        AuthManager.setAutoReconnect(state.autoReconnect)
+        // B6 (Jun 18 2026, kanban t_86e9be9b): persist theme choice so it
+        // survives a cold start (was previously dropped on save()).
+        AuthManager.setThemePreference(state.themePreference)
        ApiClient.rebuild()

        _uiState.update { it.copy(isSaved = true, testResult = null) }