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 <hy4ri@users.noreply.github.com>
This commit is contained in:
1 parent
d616c56176
commit
3605ca7537
4 files changed
+39
-9
No files matched your search
@@ -7,6 +7,9 @@ import androidx.room.Query
|
|||||||
|
|
||||||
@Dao
|
@Dao
|
||||||
interface ChatMessageDao {
|
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")
|
@Query("SELECT * FROM chat_messages WHERE session_id = :sessionId ORDER BY timestamp ASC")
|
||||||
suspend fun getMessagesForSession(sessionId: String): List<ChatMessageEntity>
|
suspend fun getMessagesForSession(sessionId: String): List<ChatMessageEntity>
|
||||||
|
|
||||||
|
|||||||
@@ -48,8 +48,6 @@ open class NotificationReplyReceiver : BroadcastReceiver() {
|
|||||||
val sessionId = intent.getStringExtra(EXTRA_SESSION_ID)
|
val sessionId = intent.getStringExtra(EXTRA_SESSION_ID)
|
||||||
|
|
||||||
if (!replyText.isNullOrBlank() && !sessionId.isNullOrBlank()) {
|
if (!replyText.isNullOrBlank() && !sessionId.isNullOrBlank()) {
|
||||||
HermesWsClient.sendMessage(sessionId, replyText)
|
|
||||||
|
|
||||||
val pendingResult = goAsyncCompat()
|
val pendingResult = goAsyncCompat()
|
||||||
kotlinx.coroutines.CoroutineScope(kotlinx.coroutines.Dispatchers.IO).launch {
|
kotlinx.coroutines.CoroutineScope(kotlinx.coroutines.Dispatchers.IO).launch {
|
||||||
try {
|
try {
|
||||||
@@ -58,6 +56,13 @@ open class NotificationReplyReceiver : BroadcastReceiver() {
|
|||||||
com.m57.hermescontrol.data.local.HermesDatabase
|
com.m57.hermescontrol.data.local.HermesDatabase
|
||||||
.get(context)
|
.get(context)
|
||||||
val dao = db.chatMessageDao()
|
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 =
|
val entity =
|
||||||
com.m57.hermescontrol.data.local.ChatMessageEntity(
|
com.m57.hermescontrol.data.local.ChatMessageEntity(
|
||||||
id =
|
id =
|
||||||
@@ -70,18 +75,17 @@ open class NotificationReplyReceiver : BroadcastReceiver() {
|
|||||||
timestamp = System.currentTimeMillis(),
|
timestamp = System.currentTimeMillis(),
|
||||||
)
|
)
|
||||||
dao.upsert(entity)
|
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) {
|
} 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 {
|
} finally {
|
||||||
pendingResult.finish()
|
pendingResult.finish()
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
val repliedNotification = buildReplyNotification(context)
|
|
||||||
|
|
||||||
val manager = context.getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager
|
|
||||||
manager.notify(ChatNotificationService.PENDING_NOTIFICATION_ID, repliedNotification)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -84,6 +84,7 @@ class NotificationReplyReceiverTest {
|
|||||||
upsertCallCount++
|
upsertCallCount++
|
||||||
Unit
|
Unit
|
||||||
}
|
}
|
||||||
|
coEvery { mockDao.sessionExists(any<String>()) } returns true
|
||||||
mockDb = mockk(relaxed = true)
|
mockDb = mockk(relaxed = true)
|
||||||
every { mockDb.chatMessageDao() } returns mockDao
|
every { mockDb.chatMessageDao() } returns mockDao
|
||||||
HermesDatabase.setForTest(mockDb)
|
HermesDatabase.setForTest(mockDb)
|
||||||
@@ -123,6 +124,7 @@ class NotificationReplyReceiverTest {
|
|||||||
givenValidReply("session-abc", "Hello")
|
givenValidReply("session-abc", "Hello")
|
||||||
|
|
||||||
receiver.onReceive(mockContext, mockIntent)
|
receiver.onReceive(mockContext, mockIntent)
|
||||||
|
Thread.sleep(500) // Need to wait for coroutine
|
||||||
|
|
||||||
verify { HermesWsClient.sendMessage("session-abc", "Hello") }
|
verify { HermesWsClient.sendMessage("session-abc", "Hello") }
|
||||||
}
|
}
|
||||||
@@ -153,6 +155,7 @@ class NotificationReplyReceiverTest {
|
|||||||
givenValidReply("session-abc", "Hello")
|
givenValidReply("session-abc", "Hello")
|
||||||
|
|
||||||
receiver.onReceive(mockContext, mockIntent)
|
receiver.onReceive(mockContext, mockIntent)
|
||||||
|
Thread.sleep(500) // Need to wait for coroutine
|
||||||
|
|
||||||
verify {
|
verify {
|
||||||
mockNotificationManager.notify(
|
mockNotificationManager.notify(
|
||||||
@@ -235,6 +238,26 @@ class NotificationReplyReceiverTest {
|
|||||||
assertFalse("upsert should NOT be called for null remote input", upsertCallCount > 0)
|
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
|
@Test
|
||||||
fun `Room failure logs error and finishes pending result`() {
|
fun `Room failure logs error and finishes pending result`() {
|
||||||
givenValidReply("session-abc", "Hello")
|
givenValidReply("session-abc", "Hello")
|
||||||
|
|||||||
@@ -377,7 +377,7 @@ class ConnectViewModelTest {
|
|||||||
every { android.util.Base64.decode(any<String>(), any()) } throws IllegalArgumentException("bad base64")
|
every { android.util.Base64.decode(any<String>(), any()) } throws IllegalArgumentException("bad base64")
|
||||||
|
|
||||||
// A string that is >= 32 chars and alphanumeric matching the raw token regex
|
// 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)
|
viewModel.onPairingString(validToken)
|
||||||
|
|
||||||
val state = viewModel.uiState.value
|
val state = viewModel.uiState.value
|
||||||
|
|||||||
Reference in new issue
Block a user