From 38339bfd9b4149c4c47bf2f3a43f101c88589583 Mon Sep 17 00:00:00 2001 From: James Rich Date: Tue, 28 Jul 2026 13:11:45 -0500 Subject: [PATCH 1/6] fix(event): honor disabled node events, gate brand URLs, observe the manifest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects found while reconciling the cross-platform spec (meshtastic/design#132) against what actually shipped. **Node-event notifications could be silently switched back on.** Connecting to event firmware set the auto-disabled flag without checking whether node events were even on, so a user who had turned them off themselves got the flag set — and the vanilla branch then re-enabled them on the next ordinary connection, discarding a preference we never changed. Only claim the restore when we are the one turning them off. **Manifest URLs reached the URI handler unchecked.** Event links went straight to openUri() after only a blank check, and iconUrl straight to the image loader. The manifest is first-party but arrives over the network, and a URI handler honors whatever scheme it is given. Require an absolute https URL with a real host, rejecting cleartext, protocol-relative, non-http schemes, and userinfo that lets a hostile host masquerade as a trusted one. **A manifest refresh never reached branding already on screen.** eventEdition resolved the edition with a suspending one-shot inside combine(), so metadata landing after connection sat in the cache until the user reconnected. Add observeEdition() backed by a Room Flow and flatMapLatest onto it, so a refresh re-emits into the live UI. Co-Authored-By: Claude Opus 5 --- .../EventFirmwareEditionLocalDataSource.kt | 7 +++ .../data/manager/MeshConfigFlowManagerImpl.kt | 14 +++--- .../repository/EventFirmwareRepositoryImpl.kt | 23 +++++++++- .../manager/MeshConfigFlowManagerImplTest.kt | 28 ++++++++++++ .../EventFirmwareRepositoryImplTest.kt | 37 ++++++++++++++++ .../database/dao/EventFirmwareEditionDao.kt | 9 ++++ .../repository/EventFirmwareRepository.kt | 10 +++++ .../core/ui/component/EventInfoSheet.kt | 4 +- .../core/ui/util/LocalEventBranding.kt | 29 ++++++++++-- .../core/ui/viewmodel/UIViewModel.kt | 18 +++++--- .../core/ui/util/EventBrandingTest.kt | 44 +++++++++++++++++++ 11 files changed, 204 insertions(+), 19 deletions(-) diff --git a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/EventFirmwareEditionLocalDataSource.kt b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/EventFirmwareEditionLocalDataSource.kt index 891ed6efc1..095d2e54b9 100644 --- a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/EventFirmwareEditionLocalDataSource.kt +++ b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/EventFirmwareEditionLocalDataSource.kt @@ -16,6 +16,9 @@ */ package org.meshtastic.core.data.datasource +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.withContext import org.koin.core.annotation.Single import org.meshtastic.core.database.DatabaseProvider @@ -35,6 +38,10 @@ class EventFirmwareEditionLocalDataSource( suspend fun getByEdition(edition: String): EventFirmwareEditionEntity? = withContext(dispatchers.io) { dao.getByEdition(edition) } + @OptIn(ExperimentalCoroutinesApi::class) + fun observeByEdition(edition: String): Flow = + dbManager.currentDb.flatMapLatest { db -> db.eventFirmwareEditionDao().observeByEdition(edition) } + suspend fun upsertAll(editions: List) { withContext(dispatchers.io) { dbManager.withDb { it.eventFirmwareEditionDao().upsertAll(editions) } } } diff --git a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImpl.kt b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImpl.kt index 7640e2285a..ed2da0c69f 100644 --- a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImpl.kt +++ b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImpl.kt @@ -494,16 +494,18 @@ class MeshConfigFlowManagerImpl( } private fun applyEventFirmwareNotificationDefaults(edition: FirmwareEdition) { + val autoDisabled = notificationPrefs.nodeEventsAutoDisabledForEvent.value if (edition != FirmwareEdition.VANILLA) { - if (!notificationPrefs.nodeEventsAutoDisabledForEvent.value) { + // Only claim the restore if node events were actually on. Setting the flag when the user had already + // turned them off would make the vanilla branch below switch them back on — silently discarding a + // preference we never changed. + if (!autoDisabled && notificationPrefs.nodeEventsEnabled.value) { notificationPrefs.setNodeEventsEnabled(false) notificationPrefs.setNodeEventsAutoDisabledForEvent(true) } - } else { - if (notificationPrefs.nodeEventsAutoDisabledForEvent.value) { - notificationPrefs.setNodeEventsEnabled(true) - notificationPrefs.setNodeEventsAutoDisabledForEvent(false) - } + } else if (autoDisabled) { + notificationPrefs.setNodeEventsEnabled(true) + notificationPrefs.setNodeEventsAutoDisabledForEvent(false) } } } diff --git a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImpl.kt b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImpl.kt index bf3835360c..1132193ded 100644 --- a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImpl.kt +++ b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImpl.kt @@ -17,6 +17,11 @@ package org.meshtastic.core.data.repository import co.touchlab.kermit.Logger +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.emitAll +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.flowOn +import kotlinx.coroutines.flow.map import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext @@ -79,12 +84,26 @@ class EventFirmwareRepositoryImpl( override suspend fun getEdition(editionName: String): EventFirmwareEdition? = withContext(dispatchers.io) { ensureSeeded() + maybeRefresh(maxWaitMs = NETWORK_REFRESH_TIMEOUT_MS) + localDataSource.getByEdition(editionName)?.asExternalModel() + } + + override fun observeEdition(editionName: String): Flow = flow { + ensureSeeded() + // Don't wait — the emissions below carry whatever the refresh writes, so blocking would only delay the + // cached value the caller could already be showing. + maybeRefresh(maxWaitMs = 0) + emitAll(localDataSource.observeByEdition(editionName).map { it?.asExternalModel() }) + } + .flowOn(dispatchers.io) + + /** Refreshes if the cache is stale and the retry cooldown has elapsed. See [lastAttemptMillis] for the cooldown. */ + private suspend fun maybeRefresh(maxWaitMs: Long) { val stale = nowMillis - lastRefreshMillis > CACHE_EXPIRATION_TIME_MS val retryCooldownElapsed = nowMillis - lastAttemptMillis > REFRESH_RETRY_COOLDOWN_MS if (stale && retryCooldownElapsed) { - refresher.refresh(maxWaitMs = NETWORK_REFRESH_TIMEOUT_MS) + refresher.refresh(maxWaitMs = maxWaitMs) } - localDataSource.getByEdition(editionName)?.asExternalModel() } /** Seeds the table from the bundled snapshot if empty (fresh install, data clear). */ diff --git a/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImplTest.kt b/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImplTest.kt index 4d227c9a40..6a37ca7ff5 100644 --- a/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImplTest.kt +++ b/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImplTest.kt @@ -867,6 +867,34 @@ class MeshConfigFlowManagerImplTest { verify(mode = VerifyMode.not) { notificationPrefs.setNodeEventsAutoDisabledForEvent(any()) } } + @Test + fun `handleMyInfo does not claim the restore when node events were already off`() = testScope.runTest { + // The user turned node events off themselves. We never disabled them, so we must not flag a restore — doing + // so + // would make the next vanilla connection switch them back on and discard the user's choice. + every { notificationPrefs.nodeEventsAutoDisabledForEvent } returns MutableStateFlow(false) + every { notificationPrefs.nodeEventsEnabled } returns MutableStateFlow(false) + + handleMyInfo(protoMyNodeInfo.copy(firmware_edition = FirmwareEdition.DEFCON)) + advanceUntilIdle() + + verify(mode = VerifyMode.not) { notificationPrefs.setNodeEventsEnabled(any()) } + verify(mode = VerifyMode.not) { notificationPrefs.setNodeEventsAutoDisabledForEvent(any()) } + } + + @Test + fun `handleMyInfo still auto-disables when node events are on`() = testScope.runTest { + // Guard against over-tightening the check above into never auto-disabling at all. + every { notificationPrefs.nodeEventsAutoDisabledForEvent } returns MutableStateFlow(false) + every { notificationPrefs.nodeEventsEnabled } returns MutableStateFlow(true) + + handleMyInfo(protoMyNodeInfo.copy(firmware_edition = FirmwareEdition.DEFCON)) + advanceUntilIdle() + + verify { notificationPrefs.setNodeEventsEnabled(false) } + verify { notificationPrefs.setNodeEventsAutoDisabledForEvent(true) } + } + // ---------- onHandshakeProgress ---------- @Test diff --git a/core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt b/core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt index bdde1a959f..c7c0655bf4 100644 --- a/core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt +++ b/core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt @@ -17,12 +17,17 @@ package org.meshtastic.core.data.repository import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout import kotlinx.serialization.json.Json import okio.Buffer import okio.Source import org.meshtastic.core.data.datasource.BundledAssetReader import org.meshtastic.core.data.datasource.EventFirmwareEditionLocalDataSource +import org.meshtastic.core.database.entity.asEntity import org.meshtastic.core.di.CoroutineDispatchers import org.meshtastic.core.model.EventFirmwareBuild import org.meshtastic.core.model.EventFirmwareEdition @@ -129,6 +134,32 @@ class EventFirmwareRepositoryImplTest { assertNull(repository.getEdition("VANILLA")) } + @Test + fun observeEditionReEmitsWhenTheCacheIsRefreshed() = runBlocking { + // The point of the observable: an edition absent at connect time — because the bundled seed predates it — + // must reach the UI when the refresh lands, without the caller re-subscribing. + seed.editions = listOf(edition("HAMVENTION")) + + val emissions = mutableListOf() + val collector = launch { repository.observeEdition("DEFCON").collect { emissions += it?.displayName } } + withTimeout(EMISSION_TIMEOUT_MS) { while (emissions.isEmpty()) delay(EMISSION_POLL_MS) } + assertNull(emissions.first()) + + api.response = EventFirmwareResponse(editions = listOf(edition("DEFCON"))) + local.upsertAll(listOf(edition("DEFCON").asEntity())) + + withTimeout(EMISSION_TIMEOUT_MS) { while (emissions.last() == null) delay(EMISSION_POLL_MS) } + assertEquals("defcon", emissions.last()) + collector.cancel() + } + + @Test + fun observeEditionEmitsNullForUnknownEdition() = runBlocking { + seed.editions = listOf(edition("HAMVENTION")) + + assertNull(repository.observeEdition("VANILLA").first()) + } + @Test fun absentAssetYieldsNullWithoutCrashing() = runBlocking { seed.present = false @@ -231,4 +262,10 @@ class EventFirmwareRepositoryImplTest { assertEquals("hamvention", restarted.getEdition("HAMVENTION")?.displayName) } + + private companion object { + /** Room's invalidation tracker delivers asynchronously, so emission waits poll rather than assume immediacy. */ + private const val EMISSION_TIMEOUT_MS = 10_000L + private const val EMISSION_POLL_MS = 20L + } } diff --git a/core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/EventFirmwareEditionDao.kt b/core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/EventFirmwareEditionDao.kt index 901df227df..d472adf981 100644 --- a/core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/EventFirmwareEditionDao.kt +++ b/core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/EventFirmwareEditionDao.kt @@ -19,6 +19,7 @@ package org.meshtastic.core.database.dao import androidx.room3.Dao import androidx.room3.Query import androidx.room3.Upsert +import kotlinx.coroutines.flow.Flow import org.meshtastic.core.database.entity.EventFirmwareEditionEntity @Dao @@ -28,6 +29,14 @@ interface EventFirmwareEditionDao { @Query("SELECT * FROM event_firmware_edition WHERE edition = :edition") suspend fun getByEdition(edition: String): EventFirmwareEditionEntity? + /** + * Observes [edition], re-emitting whenever the table changes. This is what lets a background manifest refresh reach + * already-visible event branding: a one-shot read resolves once when the connection is established, so metadata + * that lands afterwards would sit in the table unseen until the user reconnected. + */ + @Query("SELECT * FROM event_firmware_edition WHERE edition = :edition") + fun observeByEdition(edition: String): Flow + /** * Deletes rows whose edition is not in [keep]. WARNING: `NOT IN ()` is always true in SQLite, so an **empty** * [keep] deletes every row — call sites must guard against passing an empty list (see diff --git a/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/EventFirmwareRepository.kt b/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/EventFirmwareRepository.kt index 94fc5d0550..8a2d1fcc2e 100644 --- a/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/EventFirmwareRepository.kt +++ b/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/EventFirmwareRepository.kt @@ -16,10 +16,20 @@ */ package org.meshtastic.core.repository +import kotlinx.coroutines.flow.Flow import org.meshtastic.core.model.EventFirmwareEdition /** Event-firmware display metadata, seeded from the bundled `event_firmware.json` snapshot. */ interface EventFirmwareRepository { /** Metadata for [editionName] (a `FirmwareEdition` enum name), or `null` if it is not a known event edition. */ suspend fun getEdition(editionName: String): EventFirmwareEdition? + + /** + * Metadata for [editionName], re-emitting when the cache is refreshed from the network. + * + * Prefer this over [getEdition] for anything long-lived on screen. A one-shot read resolves once — when the device + * connects — so an edition added to the hosted manifest after that point stays invisible until the user reconnects. + * The flow triggers a refresh on collection and then re-emits whatever lands. + */ + fun observeEdition(editionName: String): Flow } diff --git a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/EventInfoSheet.kt b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/EventInfoSheet.kt index ab64c3db03..3dfa00f756 100644 --- a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/EventInfoSheet.kt +++ b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/EventInfoSheet.kt @@ -66,6 +66,7 @@ import org.meshtastic.core.ui.util.EventBrandingIcon import org.meshtastic.core.ui.util.accentColorOrNull import org.meshtastic.core.ui.util.brandHighlightOrNull import org.meshtastic.core.ui.util.brandPalette +import org.meshtastic.core.ui.util.safeLinks /** * Bottom sheet shown when the user taps the event branding in [MainAppBar]. Surfaces the event metadata the bundled @@ -125,7 +126,8 @@ private fun EventDetails(edition: EventFirmwareEdition, palette: List, on edition.location?.takeIf { it.isNotBlank() }?.let { InfoRow(MeshtasticIcons.Place, it, iconTint) } dateRange(edition)?.let { InfoRow(MeshtasticIcons.CalendarMonth, it, iconTint) } - val links = edition.links.filter { it.url.isNotBlank() } + // safeLinks() drops anything that isn't an https URL — the manifest is remote, and these go to the URI handler. + val links = edition.safeLinks() if (links.isNotEmpty()) { HorizontalDivider() links.forEach { link -> diff --git a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt index b7f2c287c8..8e06e07cd1 100644 --- a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt +++ b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt @@ -31,6 +31,7 @@ import org.jetbrains.compose.resources.DrawableResource import org.jetbrains.compose.resources.painterResource import org.jetbrains.compose.resources.vectorResource import org.meshtastic.core.model.EventFirmwareEdition +import org.meshtastic.core.model.EventFirmwareLink import org.meshtastic.core.resources.Res import org.meshtastic.core.resources.ic_meshtastic import org.meshtastic.core.resources.img_event_defcon @@ -57,9 +58,28 @@ fun eventIconFor(editionName: String): DrawableResource? = when (editionName) { } /** - * Event branding icon: loads the hosted [EventFirmwareEdition.iconUrl] when present, falling back to the bundled - * per-edition drawable ([eventIconFor]), and finally the Meshtastic logo. The fallback painter also backs Coil's - * loading/error states so there is never an empty slot. + * Whether [url] is safe to fetch or open from event metadata: a well-formed absolute `https` URL with a host. + * + * The manifest is first-party but arrives over the network, and its URLs reach an image loader and the platform URI + * handler. A URI handler will honour whatever scheme it is given, so without this check a bad or tampered manifest + * entry could invoke arbitrary handlers on the device. Scheme comparison is case-insensitive; everything else is + * rejected, including protocol-relative (`//host`) and scheme-relative input. + */ +fun isSafeBrandUrl(url: String?): Boolean { + val trimmed = url?.trim().orEmpty() + if (!trimmed.startsWith(HTTPS_SCHEME, ignoreCase = true)) return false + val host = trimmed.removeRange(0, HTTPS_SCHEME.length).takeWhile { it != '/' && it != '?' && it != '#' } + // Reject an empty host ("https://") and userinfo ("https://user@evil.host"), which reads as a legitimate host. + return host.isNotEmpty() && '@' !in host +} + +/** Links whose URL is safe to open — see [isSafeBrandUrl]. Unsafe or blank entries are dropped, not rendered. */ +fun EventFirmwareEdition.safeLinks(): List = links.filter { isSafeBrandUrl(it.url) } + +/** + * Event branding icon: loads the hosted [EventFirmwareEdition.iconUrl] when present *and* safe to fetch, falling back + * to the bundled per-edition drawable ([eventIconFor]), and finally the Meshtastic logo. The fallback painter also + * backs Coil's loading/error states so there is never an empty slot. */ @Composable fun EventBrandingIcon( @@ -70,7 +90,7 @@ fun EventBrandingIcon( val bundled = eventIconFor(edition.edition) val fallback = bundled?.let { painterResource(it) } ?: rememberVectorPainter(vectorResource(Res.drawable.ic_meshtastic)) - val url = edition.iconUrl + val url = edition.iconUrl?.takeIf { isSafeBrandUrl(it) } if (url.isNullOrBlank()) { Image( painter = fallback, @@ -141,6 +161,7 @@ fun EventFirmwareEdition.brandPalette(): List { fun EventFirmwareEdition.brandHighlightOrNull(): Color? = parseBrandColor(theme?.colors?.accent) ?: parseBrandColor(theme?.colors?.secondary) +private const val HTTPS_SCHEME = "https://" private const val RGB_HEX_LENGTH = 6 private const val HEX_RADIX = 16 private const val RED_SHIFT = 16 diff --git a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.kt b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.kt index 7027061656..95432feec0 100644 --- a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.kt +++ b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.kt @@ -20,6 +20,7 @@ import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import androidx.navigation3.runtime.NavKey import co.touchlab.kermit.Logger +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.channels.BufferOverflow import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow @@ -29,7 +30,10 @@ import kotlinx.coroutines.flow.asFlow import kotlinx.coroutines.flow.asSharedFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.filterNotNull +import kotlinx.coroutines.flow.flatMapLatest +import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.mapNotNull @@ -81,6 +85,7 @@ import org.meshtastic.proto.SharedContact * shared contacts, channel sets, unread counts, etc.). */ @KoinViewModel +@OptIn(ExperimentalCoroutinesApi::class) @Suppress("LongParameterList", "TooManyFunctions") class UIViewModel( private val nodeDB: NodeRepository, @@ -145,13 +150,14 @@ class UIViewModel( val eventEdition: StateFlow = combine(firmwareEdition, connectionState) { edition, state -> - // combine's transform is suspending, so the repository lookup runs here directly. - if (state is ConnectionState.Connected) { - edition?.let { eventFirmwareRepository.getEdition(it.name) } - } else { - null - } + edition?.name?.takeIf { state is ConnectionState.Connected } } + .distinctUntilChanged() + // Observe rather than read once, so a manifest refresh that lands after connecting reaches the branding + // already on screen instead of waiting for a reconnect. + .flatMapLatest { editionName -> + editionName?.let { eventFirmwareRepository.observeEdition(it) } ?: flowOf(null) + } .stateInWhileSubscribed(initialValue = null) val clientNotification: StateFlow = serviceRepository.clientNotification diff --git a/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt b/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt index 9e936b568f..84d06b8a65 100644 --- a/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt +++ b/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt @@ -18,6 +18,7 @@ package org.meshtastic.core.ui.util import androidx.compose.ui.graphics.Color import org.meshtastic.core.model.EventFirmwareEdition +import org.meshtastic.core.model.EventFirmwareLink import org.meshtastic.core.model.EventFirmwareTheme import org.meshtastic.core.model.EventFirmwareThemeColors import kotlin.test.Test @@ -124,6 +125,49 @@ class EventBrandingTest { assertTrue(EventFirmwareEdition(edition = "X").brandPalette().isEmpty()) } + @Test + fun safeBrandUrlAcceptsOnlyAbsoluteHttps() { + assertTrue(isSafeBrandUrl("https://api.meshtastic.org/resource/eventFirmware/defcon34.png")) + assertTrue(isSafeBrandUrl("HTTPS://defcon.org")) // scheme is case-insensitive + assertTrue(isSafeBrandUrl(" https://defcon.org ")) // surrounding whitespace tolerated + + assertFalse(isSafeBrandUrl("http://defcon.org")) // cleartext + assertFalse(isSafeBrandUrl("//defcon.org")) // protocol-relative + assertFalse(isSafeBrandUrl("defcon.org")) // no scheme + assertFalse(isSafeBrandUrl("https://")) // no host + assertFalse(isSafeBrandUrl("https:///path")) // empty host + assertFalse(isSafeBrandUrl(null)) + assertFalse(isSafeBrandUrl("")) + } + + @Test + fun safeBrandUrlRejectsNonHttpSchemesAndUserinfo() { + // These are the cases that make an unchecked URL dangerous: the platform URI handler honours whatever scheme it + // is handed, and userinfo lets a hostile host masquerade as a trusted one in the visible prefix. + assertFalse(isSafeBrandUrl("javascript:alert(1)")) + assertFalse(isSafeBrandUrl("file:///etc/passwd")) + assertFalse(isSafeBrandUrl("intent://scan/#Intent;scheme=zxing;end")) + assertFalse(isSafeBrandUrl("meshtastic://settings")) + assertFalse(isSafeBrandUrl("https://api.meshtastic.org@evil.example/x.png")) + } + + @Test + fun safeLinksDropsUnsafeEntriesAndKeepsOrder() { + val edition = + EventFirmwareEdition( + edition = "DEFCON", + links = + listOf( + EventFirmwareLink("Event Website", "https://defcon.org"), + EventFirmwareLink("Bad scheme", "javascript:alert(1)"), + EventFirmwareLink("Cleartext", "http://defcon.org"), + EventFirmwareLink("Blank", ""), + EventFirmwareLink("Mastodon", "https://defcon.social"), + ), + ) + assertEquals(listOf("https://defcon.org", "https://defcon.social"), edition.safeLinks().map { it.url }) + } + @Test fun brandHighlightPrefersAccentThenSecondary() { val withAccent = From 7f3c14f1ac3f3e94dd871f79a273db7f483d3744 Mon Sep 17 00:00:00 2001 From: James Rich Date: Tue, 28 Jul 2026 13:35:50 -0500 Subject: [PATCH 2/6] fix(event): validate the parsed authority and bound the observable query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit review on #6499. isSafeBrandUrl only checked that the authority was non-empty and free of '@', so "https://:443" (a port with no host) and "https://ho st/x" both passed. Match the authority whole instead — a registered name or bracketed IPv6 literal plus an optional numeric port — which also rejects truncated IPv6 literals and userinfo, since none of those characters are in the permitted set. Add LIMIT 1 to observeByEdition, per the repo guideline for single-row queries. Co-Authored-By: Claude Opus 5 --- .../core/database/dao/EventFirmwareEditionDao.kt | 2 +- .../meshtastic/core/ui/util/LocalEventBranding.kt | 15 +++++++++++---- .../meshtastic/core/ui/util/EventBrandingTest.kt | 14 ++++++++++++++ 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/EventFirmwareEditionDao.kt b/core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/EventFirmwareEditionDao.kt index d472adf981..f60adf8cec 100644 --- a/core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/EventFirmwareEditionDao.kt +++ b/core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/EventFirmwareEditionDao.kt @@ -34,7 +34,7 @@ interface EventFirmwareEditionDao { * already-visible event branding: a one-shot read resolves once when the connection is established, so metadata * that lands afterwards would sit in the table unseen until the user reconnected. */ - @Query("SELECT * FROM event_firmware_edition WHERE edition = :edition") + @Query("SELECT * FROM event_firmware_edition WHERE edition = :edition LIMIT 1") fun observeByEdition(edition: String): Flow /** diff --git a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt index 8e06e07cd1..86ab7cfe2c 100644 --- a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt +++ b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt @@ -58,19 +58,23 @@ fun eventIconFor(editionName: String): DrawableResource? = when (editionName) { } /** - * Whether [url] is safe to fetch or open from event metadata: a well-formed absolute `https` URL with a host. + * Whether [url] is safe to fetch or open from event metadata: an absolute `https` URL whose authority is a valid host + * with an optional numeric port. * * The manifest is first-party but arrives over the network, and its URLs reach an image loader and the platform URI * handler. A URI handler will honour whatever scheme it is given, so without this check a bad or tampered manifest * entry could invoke arbitrary handlers on the device. Scheme comparison is case-insensitive; everything else is * rejected, including protocol-relative (`//host`) and scheme-relative input. + * + * The authority is matched whole rather than merely tested for emptiness, so a port with no host (`https://:443`), + * whitespace, a truncated IPv6 literal, and userinfo (`https://trusted.host@evil.example`, which wears a trusted + * prefix) are all rejected — none of those characters appear in the permitted set. */ fun isSafeBrandUrl(url: String?): Boolean { val trimmed = url?.trim().orEmpty() if (!trimmed.startsWith(HTTPS_SCHEME, ignoreCase = true)) return false - val host = trimmed.removeRange(0, HTTPS_SCHEME.length).takeWhile { it != '/' && it != '?' && it != '#' } - // Reject an empty host ("https://") and userinfo ("https://user@evil.host"), which reads as a legitimate host. - return host.isNotEmpty() && '@' !in host + val authority = trimmed.drop(HTTPS_SCHEME.length).takeWhile { it != '/' && it != '?' && it != '#' } + return AUTHORITY_REGEX.matches(authority) } /** Links whose URL is safe to open — see [isSafeBrandUrl]. Unsafe or blank entries are dropped, not rendered. */ @@ -162,6 +166,9 @@ fun EventFirmwareEdition.brandHighlightOrNull(): Color? = parseBrandColor(theme?.colors?.accent) ?: parseBrandColor(theme?.colors?.secondary) private const val HTTPS_SCHEME = "https://" + +/** A registered name or bracketed IPv6 literal, plus an optional numeric port. See [isSafeBrandUrl]. */ +private val AUTHORITY_REGEX = Regex("""(?:[A-Za-z0-9._~-]+|\[[0-9A-Fa-f:.]+])(?::\d+)?""") private const val RGB_HEX_LENGTH = 6 private const val HEX_RADIX = 16 private const val RED_SHIFT = 16 diff --git a/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt b/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt index 84d06b8a65..7eeb3a29f0 100644 --- a/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt +++ b/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt @@ -130,6 +130,8 @@ class EventBrandingTest { assertTrue(isSafeBrandUrl("https://api.meshtastic.org/resource/eventFirmware/defcon34.png")) assertTrue(isSafeBrandUrl("HTTPS://defcon.org")) // scheme is case-insensitive assertTrue(isSafeBrandUrl(" https://defcon.org ")) // surrounding whitespace tolerated + assertTrue(isSafeBrandUrl("https://defcon.org:8443/x.png")) // explicit port + assertTrue(isSafeBrandUrl("https://[2606:4700::1]/x.png")) // IPv6 literal assertFalse(isSafeBrandUrl("http://defcon.org")) // cleartext assertFalse(isSafeBrandUrl("//defcon.org")) // protocol-relative @@ -140,6 +142,18 @@ class EventBrandingTest { assertFalse(isSafeBrandUrl("")) } + @Test + fun safeBrandUrlRejectsMalformedAuthorities() { + // A non-empty authority is not the same as a valid host: each of these has an authority that is present but + // not a host, so the check has to match the authority whole rather than test it for emptiness. + assertFalse(isSafeBrandUrl("https://:443")) // port, no host + assertFalse(isSafeBrandUrl("https://[::1")) // unterminated IPv6 literal + assertFalse(isSafeBrandUrl("https://[]/x")) // empty IPv6 literal + assertFalse(isSafeBrandUrl("https://ho st/x")) // whitespace in host + assertFalse(isSafeBrandUrl("https://defcon.org:port")) // non-numeric port + assertFalse(isSafeBrandUrl("https://defcon.org:/x")) // empty port + } + @Test fun safeBrandUrlRejectsNonHttpSchemesAndUserinfo() { // These are the cases that make an unchecked URL dangerous: the platform URI handler honours whatever scheme it From d873e1e0894de26cbd8e20fbbb8b0b701bff947d Mon Sep 17 00:00:00 2001 From: James Rich Date: Tue, 28 Jul 2026 13:45:48 -0500 Subject: [PATCH 3/6] fix(prefs): make the event node-event default a single atomic update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit was right on #6499: the guard read nodeEventsEnabled and nodeEventsAutoDisabledForEvent and then issued two writes, and those writes are asynchronous — every NotificationPrefs setter is scope.launch { dataStore.edit } and the StateFlows are stateIn over dataStore.data, so they lag the write. The window is not a couple of instructions; it spans until the launched coroutine runs. A user toggling node events in that window can be clobbered last-write-wins, and back-to-back handleMyInfo calls read stale values. Move the whole read-decide-write into NotificationPrefs as one DataStore edit block. DataStore serializes edits, so the values examined are the same snapshot written, and the decision can no longer interleave with a user toggle. The decision table now lives in NotificationPrefsTest against a real DataStore (including the full off-by-user → event → vanilla round trip that started this); MeshConfigFlowManagerImplTest keeps only the delegation, plus a case pinning DIY_EDITION as event firmware. Co-Authored-By: Claude Opus 5 --- .../data/manager/MeshConfigFlowManagerImpl.kt | 16 +---- .../manager/MeshConfigFlowManagerImplTest.kt | 70 ++++--------------- .../notification/NotificationPrefsImpl.kt | 24 +++++++ .../notification/NotificationPrefsTest.kt | 56 +++++++++++++++ .../core/repository/AppPreferences.kt | 13 ++++ .../core/testing/FakeNotificationPrefs.kt | 14 ++++ .../DesktopNotificationManagerTest.kt | 3 + 7 files changed, 126 insertions(+), 70 deletions(-) diff --git a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImpl.kt b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImpl.kt index ed2da0c69f..d876d93f18 100644 --- a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImpl.kt +++ b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImpl.kt @@ -493,20 +493,10 @@ class MeshConfigFlowManagerImpl( null } + // Whether to silence new-node notifications is a read-decide-write over two preferences, so it lives behind one + // atomic prefs operation rather than being assembled from setter calls here. private fun applyEventFirmwareNotificationDefaults(edition: FirmwareEdition) { - val autoDisabled = notificationPrefs.nodeEventsAutoDisabledForEvent.value - if (edition != FirmwareEdition.VANILLA) { - // Only claim the restore if node events were actually on. Setting the flag when the user had already - // turned them off would make the vanilla branch below switch them back on — silently discarding a - // preference we never changed. - if (!autoDisabled && notificationPrefs.nodeEventsEnabled.value) { - notificationPrefs.setNodeEventsEnabled(false) - notificationPrefs.setNodeEventsAutoDisabledForEvent(true) - } - } else if (autoDisabled) { - notificationPrefs.setNodeEventsEnabled(true) - notificationPrefs.setNodeEventsAutoDisabledForEvent(false) - } + notificationPrefs.applyEventFirmwareNodeEventDefault(isEventFirmware = edition != FirmwareEdition.VANILLA) } } diff --git a/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImplTest.kt b/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImplTest.kt index 6a37ca7ff5..fc69295d8b 100644 --- a/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImplTest.kt +++ b/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImplTest.kt @@ -821,78 +821,34 @@ class MeshConfigFlowManagerImplTest { } // ---------- Event firmware notification defaults ---------- + // + // The decision itself (which preference wins, and when) is a single atomic prefs operation and is covered in + // NotificationPrefsTest. These only assert that the config flow delegates with the right firmware classification. @Test - fun `handleMyInfo disables node notifications for event firmware`() = testScope.runTest { - every { notificationPrefs.nodeEventsAutoDisabledForEvent } returns MutableStateFlow(false) - - val eventMyInfo = protoMyNodeInfo.copy(firmware_edition = FirmwareEdition.DEFCON) - handleMyInfo(eventMyInfo) - advanceUntilIdle() - - verify { notificationPrefs.setNodeEventsEnabled(false) } - verify { notificationPrefs.setNodeEventsAutoDisabledForEvent(true) } - } - - @Test - fun `handleMyInfo does not re-disable if already auto-disabled`() = testScope.runTest { - every { notificationPrefs.nodeEventsAutoDisabledForEvent } returns MutableStateFlow(true) - - val eventMyInfo = protoMyNodeInfo.copy(firmware_edition = FirmwareEdition.DEFCON) - handleMyInfo(eventMyInfo) - advanceUntilIdle() - - verify(mode = VerifyMode.not) { notificationPrefs.setNodeEventsEnabled(any()) } - } - - @Test - fun `handleMyInfo re-enables node notifications when vanilla firmware reconnects`() = testScope.runTest { - every { notificationPrefs.nodeEventsAutoDisabledForEvent } returns MutableStateFlow(true) - - handleMyInfo(protoMyNodeInfo) + fun `handleMyInfo applies the event node-event default for event firmware`() = testScope.runTest { + handleMyInfo(protoMyNodeInfo.copy(firmware_edition = FirmwareEdition.DEFCON)) advanceUntilIdle() - verify { notificationPrefs.setNodeEventsEnabled(true) } - verify { notificationPrefs.setNodeEventsAutoDisabledForEvent(false) } + verify { notificationPrefs.applyEventFirmwareNodeEventDefault(isEventFirmware = true) } } @Test - fun `handleMyInfo does not touch prefs for vanilla when not previously auto-disabled`() = testScope.runTest { - every { notificationPrefs.nodeEventsAutoDisabledForEvent } returns MutableStateFlow(false) - + fun `handleMyInfo applies the vanilla node-event default for vanilla firmware`() = testScope.runTest { handleMyInfo(protoMyNodeInfo) advanceUntilIdle() - verify(mode = VerifyMode.not) { notificationPrefs.setNodeEventsEnabled(any()) } - verify(mode = VerifyMode.not) { notificationPrefs.setNodeEventsAutoDisabledForEvent(any()) } + verify { notificationPrefs.applyEventFirmwareNodeEventDefault(isEventFirmware = false) } } @Test - fun `handleMyInfo does not claim the restore when node events were already off`() = testScope.runTest { - // The user turned node events off themselves. We never disabled them, so we must not flag a restore — doing - // so - // would make the next vanilla connection switch them back on and discard the user's choice. - every { notificationPrefs.nodeEventsAutoDisabledForEvent } returns MutableStateFlow(false) - every { notificationPrefs.nodeEventsEnabled } returns MutableStateFlow(false) - - handleMyInfo(protoMyNodeInfo.copy(firmware_edition = FirmwareEdition.DEFCON)) - advanceUntilIdle() - - verify(mode = VerifyMode.not) { notificationPrefs.setNodeEventsEnabled(any()) } - verify(mode = VerifyMode.not) { notificationPrefs.setNodeEventsAutoDisabledForEvent(any()) } - } - - @Test - fun `handleMyInfo still auto-disables when node events are on`() = testScope.runTest { - // Guard against over-tightening the check above into never auto-disabling at all. - every { notificationPrefs.nodeEventsAutoDisabledForEvent } returns MutableStateFlow(false) - every { notificationPrefs.nodeEventsEnabled } returns MutableStateFlow(true) - - handleMyInfo(protoMyNodeInfo.copy(firmware_edition = FirmwareEdition.DEFCON)) + fun `handleMyInfo treats DIY_EDITION as event firmware`() = testScope.runTest { + // Any non-vanilla edition counts, including the DIY catch-all — it has no branding metadata but the same + // notification noise problem. + handleMyInfo(protoMyNodeInfo.copy(firmware_edition = FirmwareEdition.DIY_EDITION)) advanceUntilIdle() - verify { notificationPrefs.setNodeEventsEnabled(false) } - verify { notificationPrefs.setNodeEventsAutoDisabledForEvent(true) } + verify { notificationPrefs.applyEventFirmwareNodeEventDefault(isEventFirmware = true) } } // ---------- onHandshakeProgress ---------- diff --git a/core/prefs/src/commonMain/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsImpl.kt b/core/prefs/src/commonMain/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsImpl.kt index b30167d7d5..89e032812d 100644 --- a/core/prefs/src/commonMain/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsImpl.kt +++ b/core/prefs/src/commonMain/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsImpl.kt @@ -61,6 +61,30 @@ class NotificationPrefsImpl( scope.launch { dataStore.edit { it[KEY_NODE_EVENTS_AUTO_DISABLED] = disabled } } } + override fun applyEventFirmwareNodeEventDefault(isEventFirmware: Boolean) { + // One edit block: DataStore serializes edits, so the values read here are the same snapshot written below. + // Reading the StateFlows and calling the setters instead would race — they lag these writes. + scope.launch { + dataStore.edit { prefs -> + val enabled = prefs[KEY_NODE_EVENTS_ENABLED] ?: true + val autoDisabled = prefs[KEY_NODE_EVENTS_AUTO_DISABLED] ?: false + when { + // Only claim the restore if node events were actually on — otherwise the vanilla branch would + // later enable a preference the user turned off themselves. + isEventFirmware && !autoDisabled && enabled -> { + prefs[KEY_NODE_EVENTS_ENABLED] = false + prefs[KEY_NODE_EVENTS_AUTO_DISABLED] = true + } + + !isEventFirmware && autoDisabled -> { + prefs[KEY_NODE_EVENTS_ENABLED] = true + prefs[KEY_NODE_EVENTS_AUTO_DISABLED] = false + } + } + } + } + } + override val lowBatteryEnabled: StateFlow = dataStore.data.map { it[KEY_LOW_BATTERY_ENABLED] ?: true }.stateIn(scope, SharingStarted.Eagerly, true) diff --git a/core/prefs/src/commonTest/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsTest.kt b/core/prefs/src/commonTest/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsTest.kt index 1ab64cbe3b..5f19e14157 100644 --- a/core/prefs/src/commonTest/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsTest.kt +++ b/core/prefs/src/commonTest/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsTest.kt @@ -85,6 +85,62 @@ class NotificationPrefsTest { assertFalse(notificationPrefs.nodeEventsEnabled.value) } + // ---------- applyEventFirmwareNodeEventDefault ---------- + + @Test + fun `event firmware disables node events and claims the restore`() = testScope.runTest { + notificationPrefs.applyEventFirmwareNodeEventDefault(isEventFirmware = true) + + assertFalse(notificationPrefs.nodeEventsEnabled.value) + assertTrue(notificationPrefs.nodeEventsAutoDisabledForEvent.value) + } + + @Test + fun `event firmware leaves an already-off preference alone and claims nothing`() = testScope.runTest { + // The user turned node events off themselves. Claiming the restore here would make the next vanilla + // connection switch them back on, discarding a choice we never made. + notificationPrefs.setNodeEventsEnabled(false) + + notificationPrefs.applyEventFirmwareNodeEventDefault(isEventFirmware = true) + + assertFalse(notificationPrefs.nodeEventsEnabled.value) + assertFalse(notificationPrefs.nodeEventsAutoDisabledForEvent.value) + } + + @Test + fun `vanilla firmware restores node events only when the restore was claimed`() = testScope.runTest { + notificationPrefs.applyEventFirmwareNodeEventDefault(isEventFirmware = true) + assertFalse(notificationPrefs.nodeEventsEnabled.value) + + notificationPrefs.applyEventFirmwareNodeEventDefault(isEventFirmware = false) + + assertTrue(notificationPrefs.nodeEventsEnabled.value) + assertFalse(notificationPrefs.nodeEventsAutoDisabledForEvent.value) + } + + @Test + fun `vanilla firmware does not enable node events the user had turned off`() = testScope.runTest { + // The full round trip of the bug this guards: off by the user, connect to event firmware, back to vanilla. + notificationPrefs.setNodeEventsEnabled(false) + + notificationPrefs.applyEventFirmwareNodeEventDefault(isEventFirmware = true) + notificationPrefs.applyEventFirmwareNodeEventDefault(isEventFirmware = false) + + assertFalse(notificationPrefs.nodeEventsEnabled.value) + assertFalse(notificationPrefs.nodeEventsAutoDisabledForEvent.value) + } + + @Test + fun `repeated event firmware connections do not re-disable a manual re-enable`() = testScope.runTest { + notificationPrefs.applyEventFirmwareNodeEventDefault(isEventFirmware = true) + // User re-enables mid-event; the restore is still claimed, so reconnects must respect their choice. + notificationPrefs.setNodeEventsEnabled(true) + + notificationPrefs.applyEventFirmwareNodeEventDefault(isEventFirmware = true) + + assertTrue(notificationPrefs.nodeEventsEnabled.value) + } + @Test fun `setting lowBatteryEnabled updates preference`() = testScope.runTest { notificationPrefs.setLowBatteryEnabled(false) diff --git a/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AppPreferences.kt b/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AppPreferences.kt index 60dc140469..c2f26b3d2c 100644 --- a/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AppPreferences.kt +++ b/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AppPreferences.kt @@ -212,6 +212,19 @@ interface NotificationPrefs { val nodeEventsAutoDisabledForEvent: StateFlow + /** + * Applies the node-event notification default for the connected firmware, as a single atomic update. + * + * The decision reads both [nodeEventsEnabled] and [nodeEventsAutoDisabledForEvent] and conditionally writes both, + * so it cannot be expressed as separate reads and setter calls: the setters are asynchronous and the StateFlows lag + * them, so a caller doing it by hand can read stale values or clobber a concurrent user toggle. + * + * On event firmware, node events are disabled and the restore is claimed — but only if they were actually on, so a + * user who had already turned them off is not re-enabled later. On vanilla firmware, a previously claimed restore + * is honored and released. + */ + fun applyEventFirmwareNodeEventDefault(isEventFirmware: Boolean) + fun setNodeEventsAutoDisabledForEvent(disabled: Boolean) val lowBatteryEnabled: StateFlow diff --git a/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeNotificationPrefs.kt b/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeNotificationPrefs.kt index 8c8825e61b..bb836d21a6 100644 --- a/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeNotificationPrefs.kt +++ b/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeNotificationPrefs.kt @@ -38,6 +38,20 @@ class FakeNotificationPrefs : NotificationPrefs { nodeEventsAutoDisabledForEvent.value = disabled } + override fun applyEventFirmwareNodeEventDefault(isEventFirmware: Boolean) { + when { + isEventFirmware && !nodeEventsAutoDisabledForEvent.value && nodeEventsEnabled.value -> { + nodeEventsEnabled.value = false + nodeEventsAutoDisabledForEvent.value = true + } + + !isEventFirmware && nodeEventsAutoDisabledForEvent.value -> { + nodeEventsEnabled.value = true + nodeEventsAutoDisabledForEvent.value = false + } + } + } + override val lowBatteryEnabled = MutableStateFlow(true) override fun setLowBatteryEnabled(enabled: Boolean) { diff --git a/desktopApp/src/test/kotlin/org/meshtastic/desktop/notification/DesktopNotificationManagerTest.kt b/desktopApp/src/test/kotlin/org/meshtastic/desktop/notification/DesktopNotificationManagerTest.kt index 80e5b82aa4..33aaebee84 100644 --- a/desktopApp/src/test/kotlin/org/meshtastic/desktop/notification/DesktopNotificationManagerTest.kt +++ b/desktopApp/src/test/kotlin/org/meshtastic/desktop/notification/DesktopNotificationManagerTest.kt @@ -66,6 +66,9 @@ class DesktopNotificationManagerTest { nodeEventsAutoDisabledForEvent.value = disabled } + // Not exercised here; these tests only care about whether a category is enabled. + override fun applyEventFirmwareNodeEventDefault(isEventFirmware: Boolean) = Unit + override fun setLowBatteryEnabled(enabled: Boolean) { lowBatteryEnabled.value = enabled } From 888cdd39f05a37fc85d91ec9c6506713d9856ad6 Mon Sep 17 00:00:00 2001 From: James Rich Date: Tue, 28 Jul 2026 15:21:48 -0500 Subject: [PATCH 4/6] fix(event): normalize brand URLs and prove the observable refreshes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the full CodeRabbit review on #6499. The validator trimmed its input but callers forwarded the *original* string, so a whitespace-padded URL passed validation and then reached Coil and the URI handler malformed. safeBrandUrlOrNull() now returns the validated form, safeLinks() carries it, and the icon uses it; isSafeBrandUrl() is the boolean wrapper. The re-emission test wrote the new edition into the cache by hand, so it proved Room re-emits but not that observeEdition() refreshes at all — it would have kept passing if maybeRefresh() were removed. The edition now arrives only via the network response, and the fetch count is asserted, so collection driving the refresh is what the test actually pins. No assertion on the first emission: these tests run on Dispatchers.Unconfined, where the refresh can complete inline before emitAll starts, so observing the seed-only null is timing rather than contract. Co-Authored-By: Claude Opus 5 --- .../EventFirmwareRepositoryImplTest.kt | 19 ++++++++++------- .../core/ui/util/LocalEventBranding.kt | 21 +++++++++++++------ .../core/ui/util/EventBrandingTest.kt | 20 ++++++++++++++++++ 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt b/core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt index c7c0655bf4..31ab060b9c 100644 --- a/core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt +++ b/core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt @@ -27,7 +27,6 @@ import okio.Buffer import okio.Source import org.meshtastic.core.data.datasource.BundledAssetReader import org.meshtastic.core.data.datasource.EventFirmwareEditionLocalDataSource -import org.meshtastic.core.database.entity.asEntity import org.meshtastic.core.di.CoroutineDispatchers import org.meshtastic.core.model.EventFirmwareBuild import org.meshtastic.core.model.EventFirmwareEdition @@ -137,19 +136,23 @@ class EventFirmwareRepositoryImplTest { @Test fun observeEditionReEmitsWhenTheCacheIsRefreshed() = runBlocking { // The point of the observable: an edition absent at connect time — because the bundled seed predates it — - // must reach the UI when the refresh lands, without the caller re-subscribing. + // must reach the UI when the refresh lands, without the caller re-subscribing. The new value therefore has to + // arrive *through the refresh path*; writing it into the cache by hand would still pass if observeEdition + // stopped refreshing at all. seed.editions = listOf(edition("HAMVENTION")) + api.response = EventFirmwareResponse(editions = listOf(edition("HAMVENTION"), edition("DEFCON"))) val emissions = mutableListOf() val collector = launch { repository.observeEdition("DEFCON").collect { emissions += it?.displayName } } - withTimeout(EMISSION_TIMEOUT_MS) { while (emissions.isEmpty()) delay(EMISSION_POLL_MS) } - assertNull(emissions.first()) - api.response = EventFirmwareResponse(editions = listOf(edition("DEFCON"))) - local.upsertAll(listOf(edition("DEFCON").asEntity())) - - withTimeout(EMISSION_TIMEOUT_MS) { while (emissions.last() == null) delay(EMISSION_POLL_MS) } + withTimeout(EMISSION_TIMEOUT_MS) { while (emissions.lastOrNull() == null) delay(EMISSION_POLL_MS) } + // DEFCON exists only in the network response, never in the seed, so observing it at all proves the value + // arrived via refresh → cache write → emission. Collection is what drove that fetch: nothing else in this test + // touches the network or the cache. assertEquals("defcon", emissions.last()) + assertEquals(1, api.eventFirmwareCalls) + // Deliberately no assertion on the *first* emission: these tests run on Dispatchers.Unconfined, where the + // refresh can complete inline before emitAll starts, so whether the seed-only null is observed is timing. collector.cancel() } diff --git a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt index 86ab7cfe2c..f02bbfb7dd 100644 --- a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt +++ b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt @@ -70,15 +70,24 @@ fun eventIconFor(editionName: String): DrawableResource? = when (editionName) { * whitespace, a truncated IPv6 literal, and userinfo (`https://trusted.host@evil.example`, which wears a trusted * prefix) are all rejected — none of those characters appear in the permitted set. */ -fun isSafeBrandUrl(url: String?): Boolean { +fun safeBrandUrlOrNull(url: String?): String? { val trimmed = url?.trim().orEmpty() - if (!trimmed.startsWith(HTTPS_SCHEME, ignoreCase = true)) return false + if (!trimmed.startsWith(HTTPS_SCHEME, ignoreCase = true)) return null val authority = trimmed.drop(HTTPS_SCHEME.length).takeWhile { it != '/' && it != '?' && it != '#' } - return AUTHORITY_REGEX.matches(authority) + // Return the trimmed form, not the caller's original: consumers must use the string that was actually validated, + // or a padded-but-otherwise-valid URL passes here and then fails as a malformed URI at the image loader. + return trimmed.takeIf { AUTHORITY_REGEX.matches(authority) } } -/** Links whose URL is safe to open — see [isSafeBrandUrl]. Unsafe or blank entries are dropped, not rendered. */ -fun EventFirmwareEdition.safeLinks(): List = links.filter { isSafeBrandUrl(it.url) } +/** Whether [url] is safe to fetch or open — see [safeBrandUrlOrNull], which callers should prefer for the value. */ +fun isSafeBrandUrl(url: String?): Boolean = safeBrandUrlOrNull(url) != null + +/** + * Links that are safe to open, carrying the normalized URL — see [safeBrandUrlOrNull]. Unsafe or blank entries are + * dropped rather than rendered and refused on tap. + */ +fun EventFirmwareEdition.safeLinks(): List = + links.mapNotNull { link -> safeBrandUrlOrNull(link.url)?.let { link.copy(url = it) } } /** * Event branding icon: loads the hosted [EventFirmwareEdition.iconUrl] when present *and* safe to fetch, falling back @@ -94,7 +103,7 @@ fun EventBrandingIcon( val bundled = eventIconFor(edition.edition) val fallback = bundled?.let { painterResource(it) } ?: rememberVectorPainter(vectorResource(Res.drawable.ic_meshtastic)) - val url = edition.iconUrl?.takeIf { isSafeBrandUrl(it) } + val url = safeBrandUrlOrNull(edition.iconUrl) if (url.isNullOrBlank()) { Image( painter = fallback, diff --git a/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt b/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt index 7eeb3a29f0..ba5445ea63 100644 --- a/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt +++ b/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt @@ -165,6 +165,26 @@ class EventBrandingTest { assertFalse(isSafeBrandUrl("https://api.meshtastic.org@evil.example/x.png")) } + @Test + fun safeBrandUrlReturnsTheValidatedFormNotTheOriginal() { + // Consumers hand this straight to the image loader / URI handler, so they must get the string that was + // validated — a padded URL that passed validation and then went out untrimmed would fail as a malformed URI. + assertEquals("https://defcon.org", safeBrandUrlOrNull(" https://defcon.org ")) + assertEquals("https://defcon.org", safeBrandUrlOrNull("https://defcon.org")) + assertNull(safeBrandUrlOrNull("http://defcon.org")) + } + + @Test + fun safeLinksNormalizesTheUrlItKeeps() { + val edition = + EventFirmwareEdition( + edition = "DEFCON", + links = listOf(EventFirmwareLink("Padded", "\n https://defcon.org/schedule \t")), + ) + assertEquals(listOf("https://defcon.org/schedule"), edition.safeLinks().map { it.url }) + assertEquals(listOf("Padded"), edition.safeLinks().map { it.label }) // label preserved + } + @Test fun safeLinksDropsUnsafeEntriesAndKeepsOrder() { val edition = From fd9385e7e271a9495f921210a84b0d5c21d566b8 Mon Sep 17 00:00:00 2001 From: James Rich Date: Tue, 28 Jul 2026 15:33:46 -0500 Subject: [PATCH 5/6] test(desktop): use core:testing's NotificationPrefs fake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses a CodeRabbit nitpick on #6499. desktopApp's notification test carried its own NotificationPrefs fake, so adding applyEventFirmwareNodeEventDefault to the interface meant writing a second no-op override here — the double-maintenance cost the repo's "shared fakes live in core:testing" rule exists to avoid. Drop the local copy and depend on core:testing. Constructor flags become setter calls, since the shared fake exposes MutableStateFlow properties. Co-Authored-By: Claude Opus 5 --- desktopApp/build.gradle.kts | 1 + .../DesktopNotificationManagerTest.kt | 45 ++----------------- 2 files changed, 4 insertions(+), 42 deletions(-) diff --git a/desktopApp/build.gradle.kts b/desktopApp/build.gradle.kts index 29c2869c42..b48bb9aa02 100644 --- a/desktopApp/build.gradle.kts +++ b/desktopApp/build.gradle.kts @@ -346,6 +346,7 @@ dependencies { implementation(libs.jna) testRuntimeOnly(libs.junit.vintage.engine) + testImplementation(projects.core.testing) testImplementation(libs.koin.test) testImplementation(libs.kotlinx.coroutines.test) testImplementation(kotlin("test")) diff --git a/desktopApp/src/test/kotlin/org/meshtastic/desktop/notification/DesktopNotificationManagerTest.kt b/desktopApp/src/test/kotlin/org/meshtastic/desktop/notification/DesktopNotificationManagerTest.kt index 33aaebee84..ea2447c45b 100644 --- a/desktopApp/src/test/kotlin/org/meshtastic/desktop/notification/DesktopNotificationManagerTest.kt +++ b/desktopApp/src/test/kotlin/org/meshtastic/desktop/notification/DesktopNotificationManagerTest.kt @@ -17,12 +17,11 @@ package org.meshtastic.desktop.notification import kotlinx.coroutines.CoroutineStart -import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch import kotlinx.coroutines.test.runTest import org.meshtastic.core.repository.Notification -import org.meshtastic.core.repository.NotificationPrefs +import org.meshtastic.core.testing.FakeNotificationPrefs import org.meshtastic.desktop.DesktopNotificationManager import kotlin.test.Test import kotlin.test.assertEquals @@ -43,44 +42,6 @@ class DesktopNotificationManagerTest { } } - /** Simple [NotificationPrefs] with all categories enabled by default. */ - private class FakeNotificationPrefs( - messages: Boolean = true, - nodeEvents: Boolean = true, - lowBattery: Boolean = true, - ) : NotificationPrefs { - override val messagesEnabled = MutableStateFlow(messages) - override val nodeEventsEnabled = MutableStateFlow(nodeEvents) - override val nodeEventsAutoDisabledForEvent = MutableStateFlow(false) - override val lowBatteryEnabled = MutableStateFlow(lowBattery) - - override fun setMessagesEnabled(enabled: Boolean) { - messagesEnabled.value = enabled - } - - override fun setNodeEventsEnabled(enabled: Boolean) { - nodeEventsEnabled.value = enabled - } - - override fun setNodeEventsAutoDisabledForEvent(disabled: Boolean) { - nodeEventsAutoDisabledForEvent.value = disabled - } - - // Not exercised here; these tests only care about whether a category is enabled. - override fun applyEventFirmwareNodeEventDefault(isEventFirmware: Boolean) = Unit - - override fun setLowBatteryEnabled(enabled: Boolean) { - lowBatteryEnabled.value = enabled - } - - override val geofenceAlertOptIns = MutableStateFlow>(emptySet()) - - override fun setGeofenceAlertOptIn(waypointId: Int, enabled: Boolean) { - geofenceAlertOptIns.value = - geofenceAlertOptIns.value.toMutableSet().apply { if (enabled) add(waypointId) else remove(waypointId) } - } - } - @Test fun `dispatch sends to native sender and reports success when enabled`() = runTest { val sender = FakeNativeSender() @@ -96,7 +57,7 @@ class DesktopNotificationManagerTest { @Test fun `dispatch reports false and skips native sender when preference disabled`() = runTest { val sender = FakeNativeSender() - val manager = DesktopNotificationManager(FakeNotificationPrefs(messages = false), sender) + val manager = DesktopNotificationManager(FakeNotificationPrefs().apply { setMessagesEnabled(false) }, sender) val dispatched = manager.dispatch(Notification(title = "Msg", message = "Hi", category = Notification.Category.Message)) @@ -108,7 +69,7 @@ class DesktopNotificationManagerTest { @Test fun `alerts are always dispatched even when messages disabled`() = runTest { val sender = FakeNativeSender() - val manager = DesktopNotificationManager(FakeNotificationPrefs(messages = false), sender) + val manager = DesktopNotificationManager(FakeNotificationPrefs().apply { setMessagesEnabled(false) }, sender) val dispatched = manager.dispatch( From 9c14d5c84eb775e7b3e287414221622ec979ab6b Mon Sep 17 00:00:00 2001 From: James Rich Date: Tue, 28 Jul 2026 15:37:52 -0500 Subject: [PATCH 6/6] test(event): assert the observable with Turbine instead of a shared list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the outside-diff finding on #6499. The re-emission test collected into a MutableList from the collector coroutine while a polling loop read it from another — Room emits on its own executor, so that was a genuine data race on an ArrayList, and it also sidestepped the repo's Flow-testing guideline. Turbine's test {} replaces the list, the polling loop, and the manual timeout. The optional first null is awaited explicitly rather than polled for: whether the refresh completes before collection starts is timing under Dispatchers.Unconfined. Co-Authored-By: Claude Opus 5 --- .../EventFirmwareRepositoryImplTest.kt | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt b/core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt index 31ab060b9c..462a6dfe11 100644 --- a/core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt +++ b/core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt @@ -16,12 +16,10 @@ */ package org.meshtastic.core.data.repository +import app.cash.turbine.test import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.delay import kotlinx.coroutines.flow.first -import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking -import kotlinx.coroutines.withTimeout import kotlinx.serialization.json.Json import okio.Buffer import okio.Source @@ -46,7 +44,9 @@ import kotlin.test.AfterTest import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertNotNull import kotlin.test.assertNull +import kotlin.time.Duration.Companion.seconds class EventFirmwareRepositoryImplTest { @@ -142,18 +142,18 @@ class EventFirmwareRepositoryImplTest { seed.editions = listOf(edition("HAMVENTION")) api.response = EventFirmwareResponse(editions = listOf(edition("HAMVENTION"), edition("DEFCON"))) - val emissions = mutableListOf() - val collector = launch { repository.observeEdition("DEFCON").collect { emissions += it?.displayName } } - - withTimeout(EMISSION_TIMEOUT_MS) { while (emissions.lastOrNull() == null) delay(EMISSION_POLL_MS) } - // DEFCON exists only in the network response, never in the seed, so observing it at all proves the value - // arrived via refresh → cache write → emission. Collection is what drove that fetch: nothing else in this test - // touches the network or the cache. - assertEquals("defcon", emissions.last()) + repository.observeEdition("DEFCON").test(timeout = EMISSION_TIMEOUT) { + // The first item is null only if the refresh is still in flight when collection starts; under + // Dispatchers.Unconfined it may already have completed inline. Which of the two happens is timing, not + // contract, so accept either and assert on the value that matters. + val observed = assertNotNull(awaitItem() ?: awaitItem(), "expected the refreshed DEFCON edition") + // DEFCON exists only in the network response, never in the seed, so observing it at all proves the value + // arrived via refresh → cache write → emission. + assertEquals("defcon", observed.displayName) + cancelAndIgnoreRemainingEvents() + } + // Collection is what drove that fetch: nothing else in this test touches the network or the cache. assertEquals(1, api.eventFirmwareCalls) - // Deliberately no assertion on the *first* emission: these tests run on Dispatchers.Unconfined, where the - // refresh can complete inline before emitAll starts, so whether the seed-only null is observed is timing. - collector.cancel() } @Test @@ -267,8 +267,7 @@ class EventFirmwareRepositoryImplTest { } private companion object { - /** Room's invalidation tracker delivers asynchronously, so emission waits poll rather than assume immediacy. */ - private const val EMISSION_TIMEOUT_MS = 10_000L - private const val EMISSION_POLL_MS = 20L + /** Room's invalidation tracker plus the fake network round-trip; generous, since it only bounds a failure. */ + private val EMISSION_TIMEOUT = 10.seconds } }