From 567423336ce4666f49da6e947796b476964060fb Mon Sep 17 00:00:00 2001 From: Kayos Date: Mon, 25 May 2026 14:05:58 -0700 Subject: [PATCH] =?UTF-8?q?vc=3D37:=20round-2=20audit-fix=20sprint=20?= =?UTF-8?q?=E2=80=94=202=20CRIT=20+=2011=20HIGH=20+=204=20MED?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three round-2 Opus audits ran on the vc=35+vc=36 surface. CVE returned no new CRITs (round-1 fixes hold) but found 5 new HIGH. Code-health found 2 CRIT — both my own vc=35 regressions. Function- correctness found 5 BROKEN that the round-1 sweep missed. CRIT (from code-health round 2) R1 Subs feed avatar-backfill self-cancel loop. Subscriptions.updateAvatar emits a new _subs reference; SubsPane's LaunchedEffect(subs) reacts → refreshIfStale → refresh() → inFlight.cancel(). With N channels needing backfill the parallel-12 batch degenerated into N sequential single-channel fetches that kept aborting each other. Gated refreshIfStale on inFlight.isActive != true. R2 HistoryStore.recordAllWatches O(N²) input. The vc=35 bulk-import path collapsed N SP writes into 1 (good) but used ArrayList.add(0, item) inside a loop walking up to 50k input rows before take(50). ~1.25B shifts worst case. Rewritten: walk newest-first, filter blanks + seen IDs, stop at MAX_WATCHES. O(N) bounded by output cap. HIGH (from CVE round 2) CVE-1 PlayerScreen + VideoDetailScreen rendered raw error.message into the UI — Media3 HttpDataSource exceptions include the full request URI with sig=/pot=. User screenshots a playback error to a chat → full session credentials in the picture. Both surfaces now scrub via LogDump.scrubLine before rendering. CVE-2 SubscriptionsStore.addAll counter race — updateAndGet's lambda re-runs on CAS retry; var-outside- lambda increment double-counted. Now derives `added` from next.size - cur.size delta. CVE-3 sweepStale ran deleteRecursively() on cacheDir (up to ~256MB) on the main thread inside Application.onCreate. Moved to appScope.launch(Dispatchers.IO). CVE-MED-2 Expanded LogDump.SIGNED_PARAM_RE alternation to include n / lsig / ei / key / sparams. CVE-MED-3 PlayerScreen + VideoDetailScreen error handlers now also NowPlaying.clear() so the minibar doesn't keep claiming a dead session is loaded. CVE-MED-4 SettingsImport validates imported subscription / playlist / history URLs against IMPORT_ALLOWED_HOSTS at import time. Hostile NewPipe export can no longer smuggle attacker-controlled URLs. HIGH (from code-health round 2) R3 Store constructors hit SP + JSON-decode on main thread at Application.onCreate. Small stores (Settings, History, Subscriptions, Playlists) stay eager — sub-millisecond cost. Heavy stores (FeedCache ~225 KB, SearchCache ~150 KB) now lazy-init: their `init()` just stashes applicationContext; the actual Store + disk decode is built on first `get()`, which happens from VM IO-dispatched coroutines. R4 SearchViewModel.pool race with init coroutine. Switched pool to a plain @Volatile var (no observers anyway — LOW- R14) and exposed rebuildPool() so the cache-toggle handler and a future explicit hook can refresh it. R5 SubsPane first-paint empty flash. Seeded SubscriptionFeedUiState(loading = true) in the VM's initial state — the init coroutine always runs. R6 Dropped dead uploaderAvatar field on StreamItem. Written three places, read zero. Saved bytes in every cache entry. R7 Split mergeFromCache into pruneCacheToSubs + mergeFromCache (no side effect in the reader). Callers do prune then merge. R8 Settings cache-disable wipe now runs on Dispatchers.IO (3 SP-edit calls were on the UI thread). HIGH (from function-correctness round 2) B1 refresh() empty-channels also wipes disk cache (was in-memory only — disk orphans accumulated). B2 Settings cache OFF→ON now triggers feedVm.refresh() + searchVm.rebuildPool() so the user doesn't have to navigate away and back to repopulate. B3 SearchViewModel.submit() cache lookup was still doing SearchCache.get().load() on main (CRIT-C1 was only partial). Now uses entries.value (StateFlow snapshot). B5 SearchCacheStore.record now atomic via MutableStateFlow + updateAndGet (was load()→write() with no atomicity, so concurrent records lost entries). Q9 History.recordWatch wrapped in withContext(Dispatchers.IO). Q11 Minibar onPlayerError also stops the controller + clears media items (was leaking dead controller state). MED R10 Added comments at the 4 pre-flight NowPlaying checks noting they're optimizations, claim() is the safety guard. Prevents a future refactor recreating the round-1 race after deleting "the guard." R11 Minibar Toast continues but now layered with the controller.stop() + clearMediaItems(). CVE-MED-1 NowPlaying.claim updates metadata fields when the same URL is re-claimed (was returning false unconditionally, pinning truncated search titles over fresh-from-detail titles). Q3 onQueryChange clears state.error so a failed-submit's banner doesn't haunt the next reactive preview. Deferred to vc=38 (intentional cost/benefit): CVE HIGH-2 (Rust strawcore::search query= info-log) — needs a separate strawcore-core edit + rebuild. Logged as a follow-up. CVE HIGH-3 (DownloadManager setVisibleInDownloadsUi deprecated on API 29+) — only the direct-streaming download replacement is a full fix; that's a multi-day refactor. Q5/Q8 (SettingsImport hostile zip silent abort UX) — cosmetic dialog-title fix. Q12 (loadedUrl assignment ordering) — pre-existing, deferred again. --- buildSrc/src/main/kotlin/ProjectConfig.kt | 4 +- .../main/kotlin/com/sulkta/straw/StrawApp.kt | 36 ++++++++++-- .../com/sulkta/straw/data/FeedCacheStore.kt | 28 ++++++--- .../com/sulkta/straw/data/HistoryStore.kt | 35 +++++++---- .../com/sulkta/straw/data/SearchCacheStore.kt | 54 ++++++++++++----- .../sulkta/straw/data/SubscriptionsStore.kt | 18 +++--- .../straw/feature/channel/ChannelViewModel.kt | 1 - .../feature/dataimport/SettingsImport.kt | 26 ++++++++- .../straw/feature/detail/VideoDetailScreen.kt | 21 ++++++- .../feature/detail/VideoDetailViewModel.kt | 29 ++++++---- .../feature/feed/SubscriptionFeedViewModel.kt | 48 +++++++++++---- .../straw/feature/player/MinibarOverlay.kt | 13 ++++- .../sulkta/straw/feature/player/NowPlaying.kt | 10 +++- .../straw/feature/player/PlayerScreen.kt | 14 ++++- .../straw/feature/search/SearchViewModel.kt | 58 ++++++++++++------- .../straw/feature/settings/SettingsScreen.kt | 33 ++++++++--- .../kotlin/com/sulkta/straw/util/LogDump.kt | 12 +++- 17 files changed, 328 insertions(+), 112 deletions(-) diff --git a/buildSrc/src/main/kotlin/ProjectConfig.kt b/buildSrc/src/main/kotlin/ProjectConfig.kt index 901e66c2e..4e4aea201 100644 --- a/buildSrc/src/main/kotlin/ProjectConfig.kt +++ b/buildSrc/src/main/kotlin/ProjectConfig.kt @@ -55,6 +55,6 @@ const val NEWPIPE_APPLICATION_ID_NEW = "net.newpipe.app" // vc=19 / 0.1.0-AE — rust pipeline cutover. Extraction via // strawcore-core (Sulkta-Coop/strawcore) via the UniFFI wrapper; no // NewPipeExtractor in the runtime path. -const val STRAW_VERSION_CODE = 36 -const val STRAW_VERSION_NAME = "0.1.0-AV" +const val STRAW_VERSION_CODE = 37 +const val STRAW_VERSION_NAME = "0.1.0-AW" const val STRAW_APPLICATION_ID = "com.sulkta.straw" diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/StrawApp.kt b/strawApp/src/main/kotlin/com/sulkta/straw/StrawApp.kt index a6ae34c13..2d867add7 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/StrawApp.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/StrawApp.kt @@ -13,8 +13,19 @@ import com.sulkta.straw.data.SearchCache import com.sulkta.straw.data.Settings import com.sulkta.straw.data.Subscriptions import com.sulkta.straw.feature.dataimport.SettingsImport +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.launch class StrawApp : Application() { + /** + * App-scoped coroutine scope for one-time startup work that + * shouldn't tie up Application.onCreate. SupervisorJob so a failure + * in one launch doesn't cascade. + */ + private val appScope = CoroutineScope(SupervisorJob() + Dispatchers.IO) + override fun onCreate() { super.onCreate() // Path C-7: route Rust `log::*` calls into Android logcat under tag @@ -22,16 +33,29 @@ class StrawApp : Application() { // strawcore is silently dropped, making playback regressions invisible // from `adb logcat`. uniffi.strawcore.initLogging() - History.init(this) + // Small + universally-accessed stores: synchronous init. + // Settings is a handful of SP keys (read on first compose for + // themeMode), History caps at 50 watches + 20 searches, + // Subscriptions is a single channel list — sub-millisecond + // cost on cold cache. Settings.init(this) + History.init(this) Subscriptions.init(this) Playlists.init(this) + // vc=36 audit HIGH-R3: FeedCache (~225 KB) + SearchCache + // (~150 KB) JSON-decode at construction. Stash the + // applicationContext eagerly (cheap) so `get()` is callable + // anywhere; the actual store construction (and the disk + // decode that goes with it) is lazy. ViewModels accessing + // these on IO trigger the construction there — never on the + // main thread. FeedCache.init(this) SearchCache.init(this) - // Sweep any newpipe-import-* work-dirs left in cacheDir by a - // previous import that was killed mid-extraction. CRIT from - // the vc=34 security audit — the user's full NewPipe DB would - // otherwise live in cacheDir until the next deleteRecursively. - SettingsImport.sweepStale(this) + // vc=36 audit CVE HIGH-5: sweepStale's deleteRecursively() + // can walk ~256 MB if a previous import was LMK-killed + // mid-extraction. Strictly off the main thread. + appScope.launch { + SettingsImport.sweepStale(this@StrawApp) + } } } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/data/FeedCacheStore.kt b/strawApp/src/main/kotlin/com/sulkta/straw/data/FeedCacheStore.kt index 0464d25a3..deb46f2f6 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/data/FeedCacheStore.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/data/FeedCacheStore.kt @@ -56,16 +56,30 @@ class FeedCacheStore(context: Context) { } object FeedCache { + @Volatile private var appContext: Context? = null @Volatile private var instance: FeedCacheStore? = null + /** + * Lazy init: stash the applicationContext only. The actual Store + * (and the ~225 KB JSON decode that happens at construction) is + * deferred until the first `get()` call. Lets Application.onCreate + * return quickly while every caller still gets a valid Store — + * vc=36 audit HIGH-R3. Callers should access from a coroutine + * (IO dispatcher) where the lazy construction cost is acceptable. + */ fun init(context: Context) { - if (instance == null) { - synchronized(this) { - if (instance == null) instance = FeedCacheStore(context.applicationContext) - } - } + appContext = context.applicationContext } - fun get(): FeedCacheStore = instance - ?: error("FeedCacheStore not initialized — call FeedCache.init(context)") + fun get(): FeedCacheStore { + instance?.let { return it } + synchronized(this) { + instance?.let { return it } + val ctx = appContext + ?: error("FeedCacheStore not initialized — call FeedCache.init(context)") + val built = FeedCacheStore(ctx) + instance = built + return built + } + } } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/data/HistoryStore.kt b/strawApp/src/main/kotlin/com/sulkta/straw/data/HistoryStore.kt index 7ecc397d7..8e944d7c6 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/data/HistoryStore.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/data/HistoryStore.kt @@ -58,24 +58,37 @@ class HistoryStore(context: Context) { /** * Bulk import. Callers (currently SettingsImport) feed - * oldest→newest so the most-recent entries end up at the front - * of the capped list. Single SP write — vc=34 audit flagged the + * oldest→newest. Single SP write — vc=34 audit flagged the * per-row recordWatch in importHistory as a write-storm vector. + * + * O(N) on input size, not O(N²). The vc=35 first cut had an + * `add(0, item)` inside a loop walking up to MAX_HISTORY_IMPORT + * (~50k) entries — ArrayList shift over `merged.size` each step, + * a billion+ shifts in the worst case for a final `take(50)` that + * discards 99.9% of the work. Round-2 audit CRIT-R2. + * + * New shape: walk input newest-first (reversed; SettingsImport + * fed oldest-first), filter blanks + already-seen videoIds, take + * up to MAX_WATCHES, prepend to current. Done in one pass with + * the capped output never exceeding MAX_WATCHES. */ fun recordAllWatches(items: List) { if (items.isEmpty()) return val next = _watches.updateAndGet { current -> - val seen = current.map { it.videoId }.toMutableSet() - val merged = current.toMutableList() - for (item in items) { + val seen = HashSet(current.size + items.size) + current.forEach { seen.add(it.videoId) } + val capacity = (MAX_WATCHES - current.size).coerceAtLeast(0) + if (capacity == 0) return@updateAndGet current + val fresh = ArrayList(capacity) + // Walk newest-first; stop as soon as we have capacity. + val it = items.listIterator(items.size) + while (it.hasPrevious() && fresh.size < capacity) { + val item = it.previous() if (item.videoId.isBlank()) continue - if (item.videoId in seen) { - merged.removeAll { it.videoId == item.videoId } - } - seen.add(item.videoId) - merged.add(0, item) + if (!seen.add(item.videoId)) continue + fresh.add(item) } - merged.take(MAX_WATCHES) + (fresh + current).take(MAX_WATCHES) } sp.edit().putString(KEY_WATCHES, json.encodeToString(next)).apply() } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/data/SearchCacheStore.kt b/strawApp/src/main/kotlin/com/sulkta/straw/data/SearchCacheStore.kt index 10f5a961d..ff917b7e4 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/data/SearchCacheStore.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/data/SearchCacheStore.kt @@ -11,6 +11,11 @@ * Sized for SharedPreferences: 30 queries * 20 items each * ~250 bytes * = ~150 KB worst case. * + * Backed by a MutableStateFlow loaded once at construction — + * record()/load() are atomic against concurrent calls. vc=36 audit + * B5: the prior load()→edit()→write() pattern would clobber a + * concurrent record() with whichever happened to persist last. + * * Skips entirely when Settings.cacheEnabled is false — caller checks * the flag before reading/writing. */ @@ -20,6 +25,10 @@ package com.sulkta.straw.data import android.content.Context import android.content.SharedPreferences import com.sulkta.straw.feature.search.StreamItem +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.updateAndGet import kotlinx.serialization.Serializable import kotlinx.serialization.json.Json @@ -39,43 +48,60 @@ class SearchCacheStore(context: Context) { private val sp: SharedPreferences = context.getSharedPreferences(PREFS, Context.MODE_PRIVATE) private val json = Json { ignoreUnknownKeys = true } - fun load(): List = runCatching { - val s = sp.getString(KEY, null) ?: return emptyList() - json.decodeFromString>(s) - }.getOrDefault(emptyList()) + private val _entries = MutableStateFlow(loadFromDisk()) + val entries: StateFlow> = _entries.asStateFlow() + + /** Snapshot of the cache. Used by the reactive search filter. */ + fun load(): List = _entries.value /** * Record a freshly-fetched query result. Idempotent: a re-run of * the same query overwrites the prior entry rather than duplicating. * Oldest entries fall off when MAX_QUERIES is exceeded. + * + * Atomic via updateAndGet — concurrent records don't lose entries. */ fun record(query: String, items: List) { val q = query.trim() if (q.isEmpty() || items.isEmpty()) return val capped = items.take(MAX_ITEMS_PER_QUERY) val now = System.currentTimeMillis() - val current = load() - val without = current.filterNot { it.query.equals(q, ignoreCase = true) } - val next = (listOf(SearchCacheEntry(q, now, capped)) + without).take(MAX_QUERIES) + val next = _entries.updateAndGet { current -> + val without = current.filterNot { it.query.equals(q, ignoreCase = true) } + (listOf(SearchCacheEntry(q, now, capped)) + without).take(MAX_QUERIES) + } sp.edit().putString(KEY, json.encodeToString(next)).apply() } fun clear() { + _entries.value = emptyList() sp.edit().remove(KEY).apply() } + + private fun loadFromDisk(): List = runCatching { + val s = sp.getString(KEY, null) ?: return emptyList() + json.decodeFromString>(s) + }.getOrDefault(emptyList()) } object SearchCache { + @Volatile private var appContext: Context? = null @Volatile private var instance: SearchCacheStore? = null + /** Lazy init — see FeedCache.init for the rationale. */ fun init(context: Context) { - if (instance == null) { - synchronized(this) { - if (instance == null) instance = SearchCacheStore(context.applicationContext) - } - } + appContext = context.applicationContext } - fun get(): SearchCacheStore = instance - ?: error("SearchCacheStore not initialized — call SearchCache.init(context)") + fun get(): SearchCacheStore { + instance?.let { return it } + synchronized(this) { + instance?.let { return it } + val ctx = appContext + ?: error("SearchCacheStore not initialized — call SearchCache.init(context)") + val built = SearchCacheStore(ctx) + instance = built + return built + } + } } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/data/SubscriptionsStore.kt b/strawApp/src/main/kotlin/com/sulkta/straw/data/SubscriptionsStore.kt index e4d897927..9675b005e 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/data/SubscriptionsStore.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/data/SubscriptionsStore.kt @@ -73,20 +73,22 @@ class SubscriptionsStore(context: Context) { * caller can report an "added X" stat. */ fun addAll(refs: List): Int { - var added = 0 - val next = _subs.updateAndGet { cur -> - val byUrl = cur.associateBy { it.url }.toMutableMap() + // Derive `added` from the size delta INSTEAD of incrementing a + // var inside updateAndGet's lambda — that lambda can re-run + // under CAS contention (a concurrent toggle from the channel + // screen during a 500-row import), and a var-outside-lambda + // accumulates across retries. vc=36 audit CVE HIGH-4. + val cur = _subs.value + val next = _subs.updateAndGet { state -> + val byUrl = state.associateBy { it.url }.toMutableMap() for (r in refs) { if (r.url.isBlank()) continue - if (r.url !in byUrl) { - byUrl[r.url] = r - added++ - } + if (r.url !in byUrl) byUrl[r.url] = r } byUrl.values.toList() } persist(next) - return added + return next.size - cur.size } fun clear() { diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/channel/ChannelViewModel.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/channel/ChannelViewModel.kt index 027d0bfa9..82a3d4245 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/channel/ChannelViewModel.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/channel/ChannelViewModel.kt @@ -42,7 +42,6 @@ class ChannelViewModel : ViewModel() { title = v.title.ifBlank { "(no title)" }, uploader = v.uploader, uploaderUrl = v.uploaderUrl, - uploaderAvatar = ch.avatar, thumbnail = v.thumbnail, durationSeconds = v.durationSeconds, viewCount = v.viewCount, diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/dataimport/SettingsImport.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/dataimport/SettingsImport.kt index f2bab6d72..b35185009 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/dataimport/SettingsImport.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/dataimport/SettingsImport.kt @@ -102,6 +102,23 @@ object SettingsImport { // YouTube only — Straw doesn't extract from other services. private const val YT_SERVICE_ID = 0 + // Mirror of StrawActivity.YT_HOSTS — kept inline rather than + // imported because the activity holds the canonical copy and + // SettingsImport is the only other consumer. + // vc=36 audit CVE MED-4 — validate imported URLs at import time + // so a hostile NewPipe export can't smuggle attacker-controlled + // URLs into PlaylistStore / HistoryStore. + private val IMPORT_ALLOWED_HOSTS = setOf( + "youtube.com", "www.youtube.com", "m.youtube.com", + "music.youtube.com", "youtube-nocookie.com", "www.youtube-nocookie.com", + "youtu.be", + ) + + private fun isAllowedYtUrl(url: String): Boolean { + val host = runCatching { java.net.URI(url).host?.lowercase() }.getOrNull() ?: return false + return host in IMPORT_ALLOWED_HOSTS + } + suspend fun run(context: Context, zipUri: Uri): Result = withContext(Dispatchers.IO) { runCatching { runInner(context, zipUri) } @@ -275,6 +292,10 @@ object SettingsImport { continue } val url = c.getString(0) ?: continue + if (!isAllowedYtUrl(url)) { + skipped++ + continue + } val name = c.getString(1) ?: continue val avatar = c.getString(2) staged += ChannelRef(url = url, name = name, avatar = avatar) @@ -314,8 +335,10 @@ object SettingsImport { ).use { c -> while (c.moveToNext()) { if (c.getInt(4) != YT_SERVICE_ID) continue + val streamUrl = c.getString(0) ?: continue + if (!isAllowedYtUrl(streamUrl)) continue items += PlaylistItem( - streamUrl = c.getString(0) ?: continue, + streamUrl = streamUrl, title = c.getString(1) ?: "(no title)", thumbnail = c.getString(2), uploader = c.getString(3) ?: "", @@ -391,6 +414,7 @@ object SettingsImport { while (c.moveToNext()) { if (c.getInt(5) != YT_SERVICE_ID) continue val url = c.getString(0) ?: continue + if (!isAllowedYtUrl(url)) continue val title = c.getString(1) ?: continue val uploader = c.getString(2) ?: "" val thumb = c.getString(3) diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailScreen.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailScreen.kt index 8e6304966..d28b26527 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailScreen.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailScreen.kt @@ -99,6 +99,7 @@ import com.sulkta.straw.feature.player.LocalStrawController import com.sulkta.straw.feature.player.NowPlaying import com.sulkta.straw.feature.player.setPlayingFrom import com.sulkta.straw.feature.search.StreamItem +import com.sulkta.straw.util.LogDump import com.sulkta.straw.util.formatCount import com.sulkta.straw.util.formatViews import com.sulkta.straw.util.stripHtml @@ -346,6 +347,11 @@ fun VideoDetailScreen( // Make sure the controller is playing this video // before backing out — otherwise dropping to the // minibar would dismiss into an empty slot. + // Optimization: skip the MediaItem build if + // the controller is already on this URL. + // claim() in setPlayingFrom is the + // authoritative race-free guard — this + // check is just to avoid the work. if (NowPlaying.current.value?.streamUrl != streamUrl) { val r = state.resolved if (r == null) { @@ -397,6 +403,11 @@ fun VideoDetailScreen( Toast.makeText(context, "stream not ready", Toast.LENGTH_SHORT).show() return@OutlinedButton } + // Optimization: skip the MediaItem build if + // the controller is already on this URL. + // claim() in setPlayingFrom is the + // authoritative race-free guard — this + // check is just to avoid the work. if (NowPlaying.current.value?.streamUrl != streamUrl) { c.setPlayingFrom( streamUrl = streamUrl, @@ -714,6 +725,7 @@ private fun InlinePlayer( LaunchedEffect(controller, resolved, streamUrl) { val c = controller ?: return@LaunchedEffect val r = resolved ?: return@LaunchedEffect + // Optimization, not safety. claim() guards the race. if (NowPlaying.current.value?.streamUrl == streamUrl) return@LaunchedEffect c.setPlayingFrom( streamUrl = streamUrl, @@ -729,7 +741,14 @@ private fun InlinePlayer( val c = controller val listener = object : Player.Listener { override fun onPlayerError(error: androidx.media3.common.PlaybackException) { - playbackError = "${error.errorCodeName}: ${error.message ?: "(no message)"}" + // Scrub the message — Media3's HttpDataSource exceptions + // include the full signed URL in .message. vc=36 audit + // CVE HIGH-1. + val raw = error.message ?: "(no message)" + playbackError = "${error.errorCodeName}: ${LogDump.scrubLine(raw)}" + // Clear NowPlaying so the minibar drops the dead + // session. vc=36 audit MED-3. + NowPlaying.clear() } } c?.addListener(listener) diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt index 19e2b2fb5..61e8744b8 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt @@ -101,17 +101,23 @@ class VideoDetailViewModel : ViewModel() { val title = info.title.ifBlank { "(no title)" } val uploader = info.uploader - runCatching { - History.get().recordWatch( - WatchHistoryItem( - url = streamUrl, - videoId = videoId, - title = title, - uploader = uploader, - thumbnail = thumb, - watchedAt = 0L, - ), - ) + // Move SP write off the main coroutine — recordWatch + // JSON-encodes the watch list (up to 50 entries) + + // sp.edit().apply(). Small but synchronous; vc=36 + // audit Q9. + withContext(Dispatchers.IO) { + runCatching { + History.get().recordWatch( + WatchHistoryItem( + url = streamUrl, + videoId = videoId, + title = title, + uploader = uploader, + thumbnail = thumb, + watchedAt = 0L, + ), + ) + } } val ryd = withContext(Dispatchers.IO) { @@ -152,7 +158,6 @@ class VideoDetailViewModel : ViewModel() { title = v.title.ifBlank { "(no title)" }, uploader = v.uploader.ifBlank { uploader }, uploaderUrl = v.uploaderUrl ?: uploaderUrl, - uploaderAvatar = ch.avatar, thumbnail = v.thumbnail, durationSeconds = v.durationSeconds, viewCount = v.viewCount, diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/SubscriptionFeedViewModel.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/SubscriptionFeedViewModel.kt index e5a6408c4..2dffb22e9 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/SubscriptionFeedViewModel.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/SubscriptionFeedViewModel.kt @@ -49,7 +49,11 @@ data class SubscriptionFeedUiState( ) class SubscriptionFeedViewModel : ViewModel() { - private val _ui = MutableStateFlow(SubscriptionFeedUiState()) + // Seed loading=true: the init block always either hydrates from + // disk or fires a refresh, so the user should see the spinner + // (or cached content under it) rather than a one-frame flash of + // empty. vc=36 audit HIGH-R5. + private val _ui = MutableStateFlow(SubscriptionFeedUiState(loading = true)) val ui: StateFlow = _ui.asStateFlow() /** @@ -77,6 +81,7 @@ class SubscriptionFeedViewModel : ViewModel() { channelCache.putAll(saved) val channels = Subscriptions.get().subs.value if (channels.isNotEmpty()) { + pruneCacheToSubs(channels) _ui.value = _ui.value.copy( items = mergeFromCache(channels), lastFetchedAt = saved.values.maxOfOrNull { it.fetchedAt } ?: 0L, @@ -112,6 +117,14 @@ class SubscriptionFeedViewModel : ViewModel() { private var inFlight: Job? = null fun refreshIfStale() { + // Skip if a refresh is already in flight. vc=36 audit CRIT-R1: + // SubsPane's LaunchedEffect(subs) re-fires every time + // Subscriptions.updateAvatar emits a fresh list reference (which + // fetchChannelInto does opportunistically per channel). Without + // this gate, each per-channel avatar backfill cancels the + // parallel-12 batch and turns the refresh into N sequential + // single-channel fetches. + if (inFlight?.isActive == true) return val now = System.currentTimeMillis() val anyStale = Subscriptions.get().subs.value.any { ch -> val entry = channelCache[ch.url] @@ -125,6 +138,14 @@ class SubscriptionFeedViewModel : ViewModel() { if (channels.isEmpty()) { _ui.update { SubscriptionFeedUiState(loading = false, items = emptyList()) } channelCache.clear() + // Wipe disk too. vc=36 audit B1: previously the disk + // cache kept stale entries indefinitely after the user + // unsubscribed from everything. mergeFromCache eventually + // prunes them on the next merge, but they sat as orphans + // through cold starts in the meantime. + viewModelScope.launch(Dispatchers.IO) { + runCatching { FeedCache.get().clear() } + } return } inFlight?.cancel() @@ -142,6 +163,7 @@ class SubscriptionFeedViewModel : ViewModel() { .map { ch -> async { gate.withPermit { fetchChannelInto(ch) } } } .awaitAll() } + pruneCacheToSubs(channels) _ui.update { SubscriptionFeedUiState( loading = false, @@ -189,7 +211,6 @@ class SubscriptionFeedViewModel : ViewModel() { title = v.title.ifBlank { "(no title)" }, uploader = v.uploader.ifBlank { ch.name }, uploaderUrl = v.uploaderUrl ?: ch.url, - uploaderAvatar = freshAvatar ?: ch.avatar, thumbnail = v.thumbnail, durationSeconds = v.durationSeconds, viewCount = v.viewCount, @@ -211,18 +232,21 @@ class SubscriptionFeedViewModel : ViewModel() { } } - private fun mergeFromCache(channels: List): List { + private fun pruneCacheToSubs(channels: List) { val subUrls = channels.map { it.url }.toSet() - // Drop cache entries for unsubscribed channels so removed subs - // fall out of the feed immediately. (Has a side effect on the - // ConcurrentHashMap — kept here for atomicity vs. a separate - // pass.) channelCache.keys.toList().forEach { if (it !in subUrls) channelCache.remove(it) } - // Newest-first across channels. Pre-compute recencyScore once - // per item — vc=35 audit MED-Q15: sortedWith's comparator was - // invoking the regex twice per pair, so ~1800 regex matches on - // a 900-item merge. Pairing the score before sort drops that - // to N matches. + } + + private fun mergeFromCache(channels: List): List { + // Pure read. Caller is responsible for calling pruneCacheToSubs + // beforehand when channel-set changes matter — split here + // because the prior version's "merge" name hid a side-effecting + // prune that violated single-responsibility (vc=36 audit + // HIGH-R7). + // + // Pre-compute recencyScore once per item — vc=35 audit + // MED-Q15: sortedWith's comparator was invoking the regex + // twice per pair, so ~1800 regex matches on a 900-item merge. return channels.flatMap { ch -> channelCache[ch.url]?.items.orEmpty() } .map { it to it.recencyScore() } .sortedWith( diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/MinibarOverlay.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/MinibarOverlay.kt index a66cff9a6..596d609aa 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/MinibarOverlay.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/MinibarOverlay.kt @@ -77,15 +77,22 @@ fun MinibarOverlay( } // vc=35 audit MED-Q11: if Background-button took the user // to Home and the foreground audio fails, the only Player - // surface still listening is this minibar. Surface a Toast - // + clear NowPlaying so the minibar hides itself rather - // than claiming an already-dead session is "loaded". + // surface still listening is this minibar. + // vc=36 audit MED-3 + Q11: also stop the controller so a + // future tap doesn't seek into the dead state, AND clear + // NowPlaying so the minibar hides itself. (PlayerScreen + // and VideoDetailScreen's listeners also clear NowPlaying + // now, so this is the fallback when neither is alive.) override fun onPlayerError(error: androidx.media3.common.PlaybackException) { android.widget.Toast.makeText( ctx, "playback error: ${error.errorCodeName}", android.widget.Toast.LENGTH_LONG, ).show() + runCatching { + controller.stop() + controller.clearMediaItems() + } NowPlaying.clear() } } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/NowPlaying.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/NowPlaying.kt index 7c9cf6420..247ed09ef 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/NowPlaying.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/NowPlaying.kt @@ -53,7 +53,15 @@ object NowPlaying { fun claim(item: NowPlayingItem): Boolean { while (true) { val cur = _current.value - if (cur?.streamUrl == item.streamUrl) return false + if (cur?.streamUrl == item.streamUrl) { + // Same URL — caller doesn't need to re-prepare the + // player, but if it brought richer metadata (full + // title vs the search-result truncation, fresh + // thumbnail, updated SponsorBlock segments) refresh + // those fields. vc=36 round-2 CVE MED-1. + if (cur != item) _current.compareAndSet(cur, item) + return false + } if (_current.compareAndSet(cur, item)) return true // Lost the CAS to a concurrent writer — retry against the // fresh state. Bounded: at most a handful of competing diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt index 9a110a69c..155f471c2 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt @@ -75,6 +75,7 @@ import androidx.media3.ui.PlayerView import com.sulkta.straw.OverlayChromeColor import com.sulkta.straw.feature.detail.VideoDetailViewModel import com.sulkta.straw.net.SbSegment +import com.sulkta.straw.util.LogDump import com.sulkta.straw.util.strawLogI import kotlinx.coroutines.delay @@ -108,6 +109,7 @@ fun PlayerScreen( val r = resolved ?: return@LaunchedEffect val uploader = detail?.uploader.orEmpty() val thumbnail = detail?.thumbnail + // Optimization, not safety. claim() guards the race. if (NowPlaying.current.value?.streamUrl == streamUrl) return@LaunchedEffect c.setPlayingFrom( streamUrl = streamUrl, @@ -124,7 +126,17 @@ fun PlayerScreen( val c = controller val listener = object : Player.Listener { override fun onPlayerError(error: androidx.media3.common.PlaybackException) { - playbackError = "${error.errorCodeName}: ${error.message ?: "(no message)"}" + // Scrub the message before rendering. Media3's + // HttpDataSource exceptions embed the full request URI + // (with signature= / pot= / cpn=) in the .message + // string — visible in the on-screen error banner and + // a screenshot away from being shared. vc=36 audit + // CVE HIGH-1. + val raw = error.message ?: "(no message)" + playbackError = "${error.errorCodeName}: ${LogDump.scrubLine(raw)}" + // Also clear NowPlaying so the minibar doesn't keep + // claiming a dead session is loaded. vc=36 audit MED-3. + NowPlaying.clear() } } c?.addListener(listener) diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/search/SearchViewModel.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/search/SearchViewModel.kt index 6cb952a1f..4b938edd3 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/search/SearchViewModel.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/search/SearchViewModel.kt @@ -37,7 +37,6 @@ data class StreamItem( val title: String, val uploader: String, val uploaderUrl: String?, - val uploaderAvatar: String? = null, val thumbnail: String?, val durationSeconds: Long, val viewCount: Long, @@ -51,18 +50,36 @@ class SearchViewModel : ViewModel() { /** * In-memory snapshot of the disk corpus (saved search results + - * subs feed cache) for reactive filtering. Hydrated on Dispatchers.IO - * once at VM construction and refreshed after a successful submit. - * vc=34 audit CRIT — the previous implementation hit - * SharedPreferences + JSON-decoded ~225 KB on every keystroke, - * blocking the main thread. + * subs feed cache) for reactive filtering. Hydrated on + * Dispatchers.IO once at VM construction and refreshed after a + * successful submit. vc=34 audit CRIT-C1 — the previous + * implementation hit SharedPreferences + JSON-decoded ~225 KB on + * every keystroke, blocking the main thread. + * + * Plain @Volatile not StateFlow because nothing observes it + * (vc=36 audit LOW-R14 — the StateFlow synchronization buys + * nothing here). */ - private val pool = MutableStateFlow>(emptyList()) + @Volatile + private var pool: List = emptyList() init { + rebuildPool() + } + + /** + * Re-read both caches off the main thread and replace the pool + * snapshot. Called at construction and from Settings when the + * cache toggle flips ON (so a re-enable picks up freshly-seeded + * entries from a subsequent submit/refresh without waiting for + * process death). vc=36 audit B2/Q10. + */ + fun rebuildPool() { viewModelScope.launch { - if (Settings.get().cacheEnabled.value) { - pool.value = withContext(Dispatchers.IO) { buildPool() } + pool = if (Settings.get().cacheEnabled.value) { + withContext(Dispatchers.IO) { buildPool() } + } else { + emptyList() } } } @@ -73,10 +90,11 @@ class SearchViewModel : ViewModel() { }.distinctBy { it.url } fun onQueryChange(q: String) { - _ui.value = _ui.value.copy(query = q) - // Reactive filter: scan the in-memory `pool` as the user types. - // Pool is a List walked once per keystroke — bounded - // (~1500 items typical), no disk I/O, no JSON decode. + // Clear any prior error state when the user resumes typing — + // a failed submit's banner used to persist into the next + // reactive preview, looking like the new query had failed. + // vc=36 audit Q3. + _ui.value = _ui.value.copy(query = q, error = null) if (Settings.get().cacheEnabled.value && q.trim().length >= 2) { val matches = reactiveFilter(q.trim()) if (matches.isNotEmpty()) { @@ -84,16 +102,11 @@ class SearchViewModel : ViewModel() { results = matches, fromCache = true, loading = false, - error = null, ) } else if (_ui.value.fromCache) { - // User typed past what the cache can answer — drop the - // stale preview rather than leaving the prior query's - // results on screen pretending to match. _ui.value = _ui.value.copy(results = emptyList(), fromCache = false) } } else if (q.isBlank()) { - // Clear cached preview if the box is cleared. _ui.value = _ui.value.copy(results = emptyList(), fromCache = false) } } @@ -102,10 +115,13 @@ class SearchViewModel : ViewModel() { val q = _ui.value.query.trim() if (q.isEmpty()) return - // Cache hit on submit: show immediately, kick off a refresh - // behind it so the user gets fresh items shortly after. + // Cache hit on submit: show immediately, kick off refresh + // behind it. vc=36 audit B3 — the previous shape called + // `SearchCache.get().load()` on the main thread, doing the + // exact ~150 KB JSON decode the reactive-filter fix was + // supposed to eliminate. Now uses the StateFlow snapshot. val cached = if (Settings.get().cacheEnabled.value) { - SearchCache.get().load() + SearchCache.get().entries.value .firstOrNull { it.query.equals(q, ignoreCase = true) } ?.items } else null diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/settings/SettingsScreen.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/settings/SettingsScreen.kt index bc5e3583e..332e5b2f3 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/settings/SettingsScreen.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/settings/SettingsScreen.kt @@ -52,7 +52,10 @@ import com.sulkta.straw.data.ThemeMode import com.sulkta.straw.feature.dataimport.ImportResult import com.sulkta.straw.feature.dataimport.SettingsImport import com.sulkta.straw.feature.feed.SubscriptionFeedViewModel +import com.sulkta.straw.feature.search.SearchViewModel import com.sulkta.straw.util.LogDump +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext import kotlinx.coroutines.launch @Composable @@ -203,19 +206,31 @@ fun SettingsScreen() { fontWeight = FontWeight.SemiBold, ) val feedVm: SubscriptionFeedViewModel = viewModel() + val searchVm: SearchViewModel = viewModel() Switch( checked = cacheEnabled, onCheckedChange = { checked -> store.setCacheEnabled(checked) - if (!checked) { - // Wipe on disable — leaving stale bytes around - // defeats the purpose of opting out. - FeedCache.get().clear() - SearchCache.get().clear() - // vc=35 audit MED-C13 — wipe the in-memory - // copy too, otherwise items stayed visible - // until process death. - feedVm.clearInMemoryCache() + scope.launch { + if (!checked) { + withContext(Dispatchers.IO) { + runCatching { FeedCache.get().clear() } + runCatching { SearchCache.get().clear() } + } + feedVm.clearInMemoryCache() + // Drop the in-memory reactive-search pool + // too — without this, typing into Search + // still surfaces hits from the just-wiped + // disk cache. + searchVm.rebuildPool() + } else { + // Cache re-enabled: trigger a real refresh + // so the feed repopulates without waiting + // for the user to navigate away and back. + // vc=36 audit B2. + feedVm.refresh() + searchVm.rebuildPool() + } } }, ) diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/util/LogDump.kt b/strawApp/src/main/kotlin/com/sulkta/straw/util/LogDump.kt index d66eac8a6..d5279a144 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/util/LogDump.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/util/LogDump.kt @@ -96,12 +96,20 @@ object LogDump { * disk. Cheap line-level pass — adversarial-perfect would need a * URL parser, but the regex approach catches every documented * leak vector at zero allocation cost. + * + * Public so error-handler call sites (PlayerScreen / VideoDetail + * `playbackError`) can scrub Media3's `PlaybackException.message` + * before rendering it to the user — that string includes the full + * request URI for HttpDataSource exceptions, which would otherwise + * be a leak via screenshot. vc=36 audit CVE HIGH-1. */ - internal fun scrubLine(line: String): String { + fun scrubLine(line: String): String { var s = line // Pre-signed googlevideo URLs: keep host visible, drop path+query. s = GOOGLEVIDEO_URL_RE.replace(s, "https://.googlevideo.com/") // Any remaining signed-param shapes that snuck through other URLs. + // Expanded set vc=36 audit CVE MED-2: + n (JS-deobfuscated n-sig), + // lsig (link signature), ei (encrypted event-id), key, sparams. s = SIGNED_PARAM_RE.replace(s, "$1=") return s } @@ -110,7 +118,7 @@ object LogDump { """https?://[a-zA-Z0-9.-]*googlevideo\.com/\S+""", ) private val SIGNED_PARAM_RE = Regex( - """\b(signature|sig|pot|cpn|expire|ip|mn|ms|mo|pl)=([^&\s"']+)""", + """\b(signature|sig|pot|cpn|expire|ip|mn|ms|mo|pl|n|lsig|ei|key|sparams)=([^&\s"']+)""", RegexOption.IGNORE_CASE, ) }