From 3605ca7537bd2598575bb716c9bcb31d9f251018 Mon Sep 17 00:00:00 2001 From: M57 <41645758+Hy4ri@users.noreply.github.com> Date: Tue, 23 Jun 2026 22:25:06 +0300 Subject: [PATCH] Security: Validate session ID in NotificationReplyReceiver (#327) * Security: Validate session ID in NotificationReplyReceiver - Updated `ChatMessageDao.kt` to include `sessionExists` query. - Guarded `NotificationReplyReceiver.kt` `HermesWsClient.sendMessage` execution behind the `dao.sessionExists(sessionId)` check, preventing potential intent injection payloads against inactive/unknown sessions. - Moved message dispatch into DB IO coroutine to ensure safe execution. - Added tests in `NotificationReplyReceiverTest.kt` to assert unknown sessions ignore replies properly. Fixes #268 * Security: Validate session ID in NotificationReplyReceiver - Updated `ChatMessageDao.kt` to include `sessionExists` query. - Guarded `NotificationReplyReceiver.kt` `HermesWsClient.sendMessage` execution behind the `dao.sessionExists(sessionId)` check, preventing potential intent injection payloads against inactive/unknown sessions. - Moved message dispatch into DB IO coroutine and wrapped with a 5s timeout to ensure safe execution. - Added tests in `NotificationReplyReceiverTest.kt` to assert unknown sessions ignore replies properly. Fixes #268 * Security: Validate session ID in NotificationReplyReceiver - Updated `ChatMessageDao.kt` to include `sessionExists` query. - Guarded `NotificationReplyReceiver.kt` `HermesWsClient.sendMessage` execution behind the `dao.sessionExists(sessionId)` check, preventing potential intent injection payloads against inactive/unknown sessions. - Moved message dispatch into DB IO coroutine and wrapped with a 5s timeout to ensure safe execution. - Added tests in `NotificationReplyReceiverTest.kt` to assert unknown sessions ignore replies properly. Fixes #268 * Security: Fix fake token triggering GitGuardian - Replaced high-entropy string `ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890` with obvious fake string `dummy_token_value_that_is_long_enough_to_pass_validation_123` in `ConnectViewModelTest.kt` to fix a false positive alert during GitHub Actions GitGuardian security check. Fixes #268 CI Check failure * Security: Fix fake token triggering GitGuardian and validate session ID - Updated `ChatMessageDao.kt` to include `sessionExists` query. - Guarded `NotificationReplyReceiver.kt` `HermesWsClient.sendMessage` execution behind the `dao.sessionExists(sessionId)` check, preventing potential intent injection payloads against inactive/unknown sessions. - Moved message dispatch into DB IO coroutine and wrapped with a 5s timeout to ensure safe execution. - Added tests in `NotificationReplyReceiverTest.kt` to assert unknown sessions ignore replies properly. - Replaced high-entropy string `ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890` with obvious fake string `dummy_token_value_that_is_long_enough_to_pass_validation_123` in `ConnectViewModelTest.kt` to fix a false positive alert during GitHub Actions GitGuardian security check. Fixes #268 * Security: Fix fake token triggering GitGuardian and validate session ID - Updated `ChatMessageDao.kt` to include `sessionExists` query. - Guarded `NotificationReplyReceiver.kt` `HermesWsClient.sendMessage` execution behind the `dao.sessionExists(sessionId)` check, preventing potential intent injection payloads against inactive/unknown sessions. - Moved message dispatch into DB IO coroutine and wrapped with a 5s timeout to ensure safe execution. - Added tests in `NotificationReplyReceiverTest.kt` to assert unknown sessions ignore replies properly. - Replaced high-entropy string `ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890` with obvious fake string `dummy_token_value_that_is_long_enough_to_pass_validation_123` in `ConnectViewModelTest.kt` to fix a false positive alert during GitHub Actions GitGuardian security check. Fixes #268 * Security: Validate session ID in NotificationReplyReceiver and fix false positive secret - Updated `ChatMessageDao.kt` to include `sessionExists` query. - Guarded `NotificationReplyReceiver.kt` `HermesWsClient.sendMessage` execution behind the `dao.sessionExists(sessionId)` check, preventing potential intent injection payloads against inactive/unknown sessions. - Moved message dispatch into DB IO coroutine and wrapped with a 5s timeout to ensure safe execution. - Added tests in `NotificationReplyReceiverTest.kt` to assert unknown sessions ignore replies properly. - Replaced high-entropy string `ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890` with obvious fake string `dummy_token_value_that_is_long_enough_to_pass_validation_123` in `ConnectViewModelTest.kt` to fix a false positive alert during GitHub Actions GitGuardian security check. Fixes #268 --------- Co-authored-by: M57 --- .../data/local/ChatMessageDao.kt | 3 +++ .../notification/NotificationReplyReceiver.kt | 20 +++++++++------- .../NotificationReplyReceiverTest.kt | 23 +++++++++++++++++++ .../ui/connect/ConnectViewModelTest.kt | 2 +- 4 files changed, 39 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageDao.kt b/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageDao.kt index eadf86d8..733f54c2 100644 --- a/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageDao.kt +++ b/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageDao.kt @@ -7,6 +7,9 @@ import androidx.room.Query @Dao interface ChatMessageDao { + @Query("SELECT EXISTS(SELECT 1 FROM chat_messages WHERE session_id = :sessionId)") + suspend fun sessionExists(sessionId: String): Boolean + @Query("SELECT * FROM chat_messages WHERE session_id = :sessionId ORDER BY timestamp ASC") suspend fun getMessagesForSession(sessionId: String): List diff --git a/app/src/main/java/com/m57/hermescontrol/notification/NotificationReplyReceiver.kt b/app/src/main/java/com/m57/hermescontrol/notification/NotificationReplyReceiver.kt index d7cac71b..81e42c10 100644 --- a/app/src/main/java/com/m57/hermescontrol/notification/NotificationReplyReceiver.kt +++ b/app/src/main/java/com/m57/hermescontrol/notification/NotificationReplyReceiver.kt @@ -48,8 +48,6 @@ open class NotificationReplyReceiver : BroadcastReceiver() { val sessionId = intent.getStringExtra(EXTRA_SESSION_ID) if (!replyText.isNullOrBlank() && !sessionId.isNullOrBlank()) { - HermesWsClient.sendMessage(sessionId, replyText) - val pendingResult = goAsyncCompat() kotlinx.coroutines.CoroutineScope(kotlinx.coroutines.Dispatchers.IO).launch { try { @@ -58,6 +56,13 @@ open class NotificationReplyReceiver : BroadcastReceiver() { com.m57.hermescontrol.data.local.HermesDatabase .get(context) val dao = db.chatMessageDao() + if (!dao.sessionExists(sessionId)) { + android.util.Log.w("NotificationReply", "Ignoring reply for unknown session: $sessionId") + return@withTimeout + } + + HermesWsClient.sendMessage(sessionId, replyText) + val entity = com.m57.hermescontrol.data.local.ChatMessageEntity( id = @@ -70,18 +75,17 @@ open class NotificationReplyReceiver : BroadcastReceiver() { timestamp = System.currentTimeMillis(), ) dao.upsert(entity) + + val repliedNotification = buildReplyNotification(context) + val manager = context.getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager + manager.notify(ChatNotificationService.PENDING_NOTIFICATION_ID, repliedNotification) } } catch (e: Exception) { - android.util.Log.e("NotificationReply", "Failed to save replied message to Room DB", e) + android.util.Log.e("NotificationReply", "Failed to process reply", e) } finally { pendingResult.finish() } } - - val repliedNotification = buildReplyNotification(context) - - val manager = context.getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager - manager.notify(ChatNotificationService.PENDING_NOTIFICATION_ID, repliedNotification) } } } diff --git a/app/src/test/java/com/m57/hermescontrol/notification/NotificationReplyReceiverTest.kt b/app/src/test/java/com/m57/hermescontrol/notification/NotificationReplyReceiverTest.kt index 8d17930b..25640bc1 100644 --- a/app/src/test/java/com/m57/hermescontrol/notification/NotificationReplyReceiverTest.kt +++ b/app/src/test/java/com/m57/hermescontrol/notification/NotificationReplyReceiverTest.kt @@ -84,6 +84,7 @@ class NotificationReplyReceiverTest { upsertCallCount++ Unit } + coEvery { mockDao.sessionExists(any()) } returns true mockDb = mockk(relaxed = true) every { mockDb.chatMessageDao() } returns mockDao HermesDatabase.setForTest(mockDb) @@ -123,6 +124,7 @@ class NotificationReplyReceiverTest { givenValidReply("session-abc", "Hello") receiver.onReceive(mockContext, mockIntent) + Thread.sleep(500) // Need to wait for coroutine verify { HermesWsClient.sendMessage("session-abc", "Hello") } } @@ -153,6 +155,7 @@ class NotificationReplyReceiverTest { givenValidReply("session-abc", "Hello") receiver.onReceive(mockContext, mockIntent) + Thread.sleep(500) // Need to wait for coroutine verify { mockNotificationManager.notify( @@ -235,6 +238,26 @@ class NotificationReplyReceiverTest { assertFalse("upsert should NOT be called for null remote input", upsertCallCount > 0) } + @Test + fun `reply with unknown sessionId does nothing`() { + givenValidReply("unknown-session", "Hello") + + // Mock sessionExists to return false + coEvery { mockDao.sessionExists("unknown-session") } returns false + + receiver.onReceive(mockContext, mockIntent) + Thread.sleep(500) + + // HermesWsClient should not be called + verify(inverse = true) { HermesWsClient.sendMessage(any(), any()) } + // DB upsert should not be called + assertFalse("upsert should NOT be called for unknown sessionId", upsertCallCount > 0) + // Pending result should still be finished + verify { mockPendingResult.finish() } + // Warning should be logged + verify { android.util.Log.w("NotificationReply", "Ignoring reply for unknown session: unknown-session") } + } + @Test fun `Room failure logs error and finishes pending result`() { givenValidReply("session-abc", "Hello") diff --git a/app/src/test/java/com/m57/hermescontrol/ui/connect/ConnectViewModelTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/connect/ConnectViewModelTest.kt index 334204a6..c50f0462 100644 --- a/app/src/test/java/com/m57/hermescontrol/ui/connect/ConnectViewModelTest.kt +++ b/app/src/test/java/com/m57/hermescontrol/ui/connect/ConnectViewModelTest.kt @@ -377,7 +377,7 @@ class ConnectViewModelTest { every { android.util.Base64.decode(any(), any()) } throws IllegalArgumentException("bad base64") // A string that is >= 32 chars and alphanumeric matching the raw token regex - val validToken = "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890" + val validToken = "dummy_token_value_that_is_long_enough_to_pass_validation_123" viewModel.onPairingString(validToken) val state = viewModel.uiState.value