From a0b4ce8dc0657ee19a87a2b49210c2aac791f4cb Mon Sep 17 00:00:00 2001 From: M57 Date: Thu, 18 Jun 2026 02:17:11 +0300 Subject: [PATCH] =?UTF-8?q?fix(security,race,ux):=20close=206=20high-prior?= =?UTF-8?q?ity=20bugs=20(B1=E2=80=93B6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- app/build.gradle.kts | 2 +- .../com/m57/hermescontrol/MainActivity.kt | 6 +- .../hermescontrol/data/local/AuthManager.kt | 21 +++++ .../hermescontrol/data/remote/ApiClient.kt | 12 ++- .../hermescontrol/data/ws/HermesWsClient.kt | 76 ++++++++++++++----- .../hermescontrol/ui/chat/ChatViewModel.kt | 31 +++++++- .../m57/hermescontrol/ui/logs/LogsScreen.kt | 13 ++++ .../ui/settings/SettingsViewModel.kt | 7 ++ 8 files changed, 143 insertions(+), 25 deletions(-) diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 97d0cca7..78b923c3 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -31,7 +31,7 @@ android { buildFeatures { compose = true aidl = false - buildConfig = false + buildConfig = true shaders = false } diff --git a/app/src/main/java/com/m57/hermescontrol/MainActivity.kt b/app/src/main/java/com/m57/hermescontrol/MainActivity.kt index 5924d489..c87e88bd 100644 --- a/app/src/main/java/com/m57/hermescontrol/MainActivity.kt +++ b/app/src/main/java/com/m57/hermescontrol/MainActivity.kt @@ -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, diff --git a/app/src/main/java/com/m57/hermescontrol/data/local/AuthManager.kt b/app/src/main/java/com/m57/hermescontrol/data/local/AuthManager.kt index bf120371..8c06c465 100644 --- a/app/src/main/java/com/m57/hermescontrol/data/local/AuthManager.kt +++ b/app/src/main/java/com/m57/hermescontrol/data/local/AuthManager.kt @@ -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()}/" diff --git a/app/src/main/java/com/m57/hermescontrol/data/remote/ApiClient.kt b/app/src/main/java/com/m57/hermescontrol/data/remote/ApiClient.kt index 17ac14b4..bf9283d1 100644 --- a/app/src/main/java/com/m57/hermescontrol/data/remote/ApiClient.kt +++ b/app/src/main/java/com/m57/hermescontrol/data/remote/ApiClient.kt @@ -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 = diff --git a/app/src/main/java/com/m57/hermescontrol/data/ws/HermesWsClient.kt b/app/src/main/java/com/m57/hermescontrol/data/ws/HermesWsClient.kt index b34462fa..650c2e47 100644 --- a/app/src/main/java/com/m57/hermescontrol/data/ws/HermesWsClient.kt +++ b/app/src/main/java/com/m57/hermescontrol/data/ws/HermesWsClient.kt @@ -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) diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt index fc5f10eb..0f89c7e7 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt @@ -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}", + ) + } } } } diff --git a/app/src/main/java/com/m57/hermescontrol/ui/logs/LogsScreen.kt b/app/src/main/java/com/m57/hermescontrol/ui/logs/LogsScreen.kt index 290886f5..ef4de0d9 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/logs/LogsScreen.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/logs/LogsScreen.kt @@ -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, diff --git a/app/src/main/java/com/m57/hermescontrol/ui/settings/SettingsViewModel.kt b/app/src/main/java/com/m57/hermescontrol/ui/settings/SettingsViewModel.kt index ce74d76e..5b73c363 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/settings/SettingsViewModel.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/settings/SettingsViewModel.kt @@ -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) }