From b8325d1726c35531833bb23f235084ffe677e8f0 Mon Sep 17 00:00:00 2001 From: Kayos Date: Mon, 25 May 2026 14:56:38 -0700 Subject: [PATCH] =?UTF-8?q?vc=3D39:=20loop=20round=201/5=20=E2=80=94=209?= =?UTF-8?q?=20HIGH=20+=207=20MED=20from=203=20Opus=20round-4=20audits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three parallel Opus max-effort audits ran on vc=38. No new CRITs (the LogDump + VM-error-scrub chain held), but real new HIGHs across VMs that weren't touched in rounds 1-3 + the Rust runtime's brittle one-shot init. HIGH R4-1 Rust runtime::ensure_initialized was one-shot via Once. First-call failure (cold-boot in airplane mode, transient DNS/SELinux denial on first TLS init) consumed the Once slot and bricked the extractor for the rest of the process — every subsequent search/streamInfo/channelInfo returned DownloaderMissing forever. Replaced with AtomicBool + 5s backoff retry; success closes the door, failure retries on the next call. R4-2 VideoDetailViewModel.load tracked no inFlight Job. Activity-scoped VM is reused; tap video A → quickly tap a related-video B → both loads race, slower-finisher wins. A's resolved payload (different itags, different SB segments, wrong title chip) could render on the B detail page; recordWatch logged B while the player streamed A. Now: inFlight?.cancel() at top, fenced terminal writes with loadedUrl-stable guard. Same shape applied to ChannelViewModel (had no in-flight tracking at all). R4-3 `_ui.value = _ui.value.copy(...)` lost-write patterns survived round-3's pass in SearchViewModel + VideoDetail + Channel. Migrated all to `_ui.update {}` — same atomicity regression class round 3 was supposed to close. Submit/load terminal writes also now fence against late-arrivals. R4-4 HistoryStore.recordAllWatches reported `size_after - size_before` to SettingsImport — at a saturated store the post-state size equals the pre-state size even when 20 fresh imports landed and 20 older entries got truncated. User saw "0 watch history imported" when 30 actually landed. Now: recordAllWatches/recordAllSearches return an AtomicInteger-counted actual-fresh-added count from inside the CAS lambda; SettingsImport plumbs through to the report. R4-5 SubscriptionFeedViewModel.refresh() filtered to stale-only — user-initiated tap of Refresh was a silent no-op when every channel had been refreshed in the last 28min. Split: refresh() forces fan-out across every sub; refreshIfStale() keeps the TTL filter. Both share refreshInternal(force: Bool). R4-6 SettingsImport.importPlaylists called create() + addItem() in a loop — both write SP, and addItem walks every playlist linearly per insert. A NewPipe export with 100 playlists × 100 items = ~10k SP commits + O(N²) work. New PlaylistsStore.importPlaylist mints a single Playlist with pre-attached items, one CAS, one SP write per playlist. R4-7 VideoDetailViewModel auto-called channelInfo(uploaderUrl) on every load — no allowlist gate. An extractor-emitted non-YT uploaderUrl (poisoned related/moreFromChannel) would have triggered an arbitrary-host network call. R4-8 Similar shape: VideoDetailViewModel.recordWatch persisted whatever URL was passed to load() — extractor-emitted non-YT URLs would have survived in Recent Watches past process death. Same import-time URL allowlist now gates both. CVE-1 The reCAPTCHA error path embedded the full google.com/sorry/ URL into the user-visible banner. That URL carries `continue=` — and LogDump's scrub only matches googlevideo.com hosts. Now: strip the `continue=` param in Rust before propagating; UI shows a tappable challenge URL that still solves the rate-limit when the user opens it. MED R4-9 SettingsStore.setMaxResolution/setThemeMode/setCacheEnabled were not atomic vs toggle()'s updateAndGet pattern. Now CAS-safe + idempotent (no SP write when the value is already what's stored). R4-10 SponsorBlockClient.fetch built the URL via string concat with un-percent-encoded JSON-shaped categories list. Switched to HttpUrl.Builder().addQueryParameter() — okhttp does the right escaping. SB happens to accept the raw form today; this guards future user-typed categories. R4-11 strawHttpClient() synchronized on the interned STRAW_USER_AGENT string literal — any unrelated code that happened to lock the same literal could contend. Replaced with lazy(SYNCHRONIZED) — same one-shot init, no shared global lock. R4-12 DownloadsScreen.queryDownloads ran on the main coroutine every 1-5s. DownloadManager.query is a ContentResolver IPC + SQLite cursor walk; on devices with hundreds of historical downloads it stuttered. withContext(Dispatchers.IO). R4-13 Co-located the YT host allowlist (was inline in SettingsImport) into util/YtUrl.kt — VideoDetailViewModel now imports the same function. Future host changes are one edit. Deferred to round 2-5: R4-MED — Nav.kt has no rememberSaveable / Parcelize on Screen sealed types. Process-death loses entire back stack. Needs Parcelize plugin add + listSaver — bigger refactor. R4-HIGH — Release isMinifyEnabled = false / no R8. Needs comprehensive keep-rules for UniFFI + kotlinx-serialization before flipping safely. Holding for a dedicated round. R4-MED — LazyColumn key= missing in 5 list sites; quick win but cosmetic, won't slip into post-round-5 ship. R4-MED — collectAsStateWithLifecycle bulk-replace. R4-MED — SponsorBlock skip-loop should bind segments to controller.currentMediaItem to avoid one-tick misapply on track changes. --- buildSrc/src/main/kotlin/ProjectConfig.kt | 4 +- rust/strawcore/src/error.rs | 36 +++++- rust/strawcore/src/runtime.rs | 90 +++++++++++--- .../com/sulkta/straw/data/HistoryStore.kt | 32 ++++- .../com/sulkta/straw/data/PlaylistsStore.kt | 29 +++++ .../com/sulkta/straw/data/SettingsStore.kt | 16 ++- .../straw/feature/channel/ChannelViewModel.kt | 60 ++++++---- .../feature/dataimport/SettingsImport.kt | 51 ++++---- .../feature/detail/VideoDetailViewModel.kt | 113 +++++++++++------- .../straw/feature/download/DownloadsScreen.kt | 8 +- .../feature/feed/SubscriptionFeedViewModel.kt | 13 +- .../straw/feature/search/SearchViewModel.kt | 90 +++++++++----- .../main/kotlin/com/sulkta/straw/net/Http.kt | 26 ++-- .../sulkta/straw/net/SponsorBlockClient.kt | 17 ++- .../kotlin/com/sulkta/straw/util/YtUrl.kt | 25 ++++ 15 files changed, 443 insertions(+), 167 deletions(-) create mode 100644 strawApp/src/main/kotlin/com/sulkta/straw/util/YtUrl.kt diff --git a/buildSrc/src/main/kotlin/ProjectConfig.kt b/buildSrc/src/main/kotlin/ProjectConfig.kt index 7b8d1dc1d..926d3e84f 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 = 38 -const val STRAW_VERSION_NAME = "0.1.0-AX" +const val STRAW_VERSION_CODE = 39 +const val STRAW_VERSION_NAME = "0.1.0-AY" const val STRAW_APPLICATION_ID = "com.sulkta.straw" diff --git a/rust/strawcore/src/error.rs b/rust/strawcore/src/error.rs index 34069bb81..7b840fbe3 100644 --- a/rust/strawcore/src/error.rs +++ b/rust/strawcore/src/error.rs @@ -31,13 +31,47 @@ pub enum StrawcoreError { RequiresLogin { detail: String }, } +/// Drop the `continue=` param from a google.com/sorry/... +/// URL while leaving every other param intact. Used only for surfacing +/// recaptcha challenge URLs to the UI; keeps the URL tappable for the +/// user to solve the challenge while scrubbing the embedded +/// googlevideo signature. +fn strip_continue_param(url: &str) -> String { + let (base, query) = match url.split_once('?') { + Some(pair) => pair, + None => return url.to_owned(), + }; + let filtered: Vec<&str> = query + .split('&') + .filter(|kv| { + let key = kv.split_once('=').map(|(k, _)| k).unwrap_or(*kv); + !key.eq_ignore_ascii_case("continue") + }) + .collect(); + if filtered.is_empty() { + base.to_owned() + } else { + format!("{}?{}", base, filtered.join("&")) + } +} + impl From for StrawcoreError { fn from(e: strawcore_core::exceptions::ExtractionError) -> Self { use strawcore_core::exceptions::{ContentUnavailable, ExtractionError, NetworkError}; match e { ExtractionError::Network(NetworkError::Recaptcha { url }) => { + // Strip the `continue=` query param before propagating. + // google.com/sorry/index carries the full signed + // googlevideo URL in `continue=` so the user can be + // sent back to the stream after solving — but + // surfacing that to the UI is a credential leak via + // screenshot, and Kotlin's LogDump scrubber only + // catches googlevideo.com hosts. The challenge URL + // itself still solves without `continue=`, so the + // user can tap to unblock without leaking the + // signature/expire/pot token. Round-4 audit LOW-1. StrawcoreError::RequiresLogin { - detail: format!("reCAPTCHA at {url}"), + detail: format!("reCAPTCHA challenge: {}", strip_continue_param(&url)), } } ExtractionError::Network(NetworkError::Transport(msg)) => { diff --git a/rust/strawcore/src/runtime.rs b/rust/strawcore/src/runtime.rs index 6e1b3fbc0..258e30935 100644 --- a/rust/strawcore/src/runtime.rs +++ b/rust/strawcore/src/runtime.rs @@ -1,29 +1,83 @@ // Runtime bootstrap. Called once from Kotlin's StrawApp.onCreate via -// init_logging(). Wires the strawcore-core Downloader + Localization -// singleton so the extractor calls have an HTTP client to use. +// init_logging(), and again before every strawcore call. Wires the +// strawcore-core Downloader + Localization singleton so the extractor +// has an HTTP client to use. +// +// Round-4 audit HIGH-1: the prior shape used `Once::call_once` and +// silently swallowed errors. If the FIRST call ran while the network +// stack wasn't ready (cold boot in airplane mode, SELinux denial on +// first TLS init, transient resolver failure), the Once slot was +// consumed, NewPipe::init_full never ran, and every subsequent +// search/streamInfo/channelInfo returned DownloaderMissing for the +// rest of the process lifetime. +// +// New shape: use an AtomicBool to track success. Only "success" closes +// the door. On failure we retry — rate-limited so a persistently-broken +// network doesn't hammer reqwest::Client::new() on every call. -use std::sync::{Arc, Once}; +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; +use std::sync::{Arc, Mutex}; +use std::time::{SystemTime, UNIX_EPOCH}; use strawcore_core::downloader::ReqwestDownloader; use strawcore_core::localization::{ContentCountry, Localization}; use strawcore_core::newpipe::NewPipe; -static INIT: Once = Once::new(); +static INITIALIZED: AtomicBool = AtomicBool::new(false); +static LAST_ATTEMPT_MS: AtomicU64 = AtomicU64::new(0); +// Guards the actual init attempt so concurrent calls don't all try +// to build the downloader in parallel; serial retry is the goal. +static INIT_LOCK: Mutex<()> = Mutex::new(()); + +/// Min ms between retries when init has failed. 5s — enough that a +/// hot loop of failed searches doesn't pin a CPU on reqwest setup, +/// short enough that a user who toggled airplane mode off recovers +/// within one tap. +const RETRY_BACKOFF_MS: u64 = 5_000; + +fn now_ms() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_millis() as u64) + .unwrap_or(0) +} pub fn ensure_initialized() { - INIT.call_once(|| { - match ReqwestDownloader::new() { - Ok(dl) => { - NewPipe::init_full( - Arc::new(dl), - Localization::default(), - ContentCountry::default(), - ); - log::info!("strawcore-core: downloader + localization initialized"); - } - Err(e) => { - log::error!("strawcore-core: failed to build downloader: {e}"); - } + if INITIALIZED.load(Ordering::Acquire) { + return; + } + // Backoff check OUTSIDE the lock — avoids serializing every + // already-throttled caller on a single mutex. + let last = LAST_ATTEMPT_MS.load(Ordering::Acquire); + let now = now_ms(); + if last != 0 && now.saturating_sub(last) < RETRY_BACKOFF_MS { + return; + } + let _guard = match INIT_LOCK.lock() { + Ok(g) => g, + Err(p) => p.into_inner(), + }; + // Re-check under the lock — another thread may have just + // succeeded while we were waiting. + if INITIALIZED.load(Ordering::Acquire) { + return; + } + LAST_ATTEMPT_MS.store(now_ms(), Ordering::Release); + match ReqwestDownloader::new() { + Ok(dl) => { + NewPipe::init_full( + Arc::new(dl), + Localization::default(), + ContentCountry::default(), + ); + INITIALIZED.store(true, Ordering::Release); + log::info!("strawcore-core: downloader + localization initialized"); } - }); + Err(e) => { + // Don't surface the underlying error string verbatim — + // it can embed URLs / hosts. + log::error!("strawcore-core: downloader init failed (will retry on next call)"); + let _ = e; + } + } } 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 30aca3d0c..a785c58c2 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/data/HistoryStore.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/data/HistoryStore.kt @@ -71,10 +71,24 @@ class HistoryStore(context: Context) { * reference equality after updateAndGet's no-op return) so a * spam-import on an already-up-to-date store doesn't thrash disk. */ - fun recordAllWatches(items: List) { - if (items.isEmpty()) return + /** + * Returns the number of fresh items actually folded into the + * store on this call (counts new videoIds; duplicates of + * already-recorded entries don't count). Round-4 audit HIGH-7 — + * SettingsImport previously reported `size_after - size_before` + * which lies when the store was at MAX_WATCHES (post-state can + * be 50 = pre-state even when 20 imports landed and 20 older + * locals were truncated to make room). + */ + fun recordAllWatches(items: List): Int { + if (items.isEmpty()) return 0 val before = _watches.value + val counter = java.util.concurrent.atomic.AtomicInteger(0) val next = _watches.updateAndGet { current -> + // Reset the counter inside the CAS lambda so a retry + // doesn't accumulate across attempts — same shape as + // SubscriptionsStore.addAll's vc=37 round-3 fix. + counter.set(0) val seen = HashSet(current.size + items.size) current.forEach { seen.add(it.videoId) } // Build the import list newest-first. Capped at @@ -87,6 +101,7 @@ class HistoryStore(context: Context) { if (item.videoId.isBlank()) continue if (!seen.add(item.videoId)) continue fresh.add(item) + counter.incrementAndGet() } if (fresh.isEmpty()) return@updateAndGet current // Combine + cap. take() truncates older `current` entries @@ -96,6 +111,7 @@ class HistoryStore(context: Context) { if (next !== before) { sp.edit().putString(KEY_WATCHES, json.encodeToString(next)).apply() } + return counter.get() } /** @@ -105,10 +121,16 @@ class HistoryStore(context: Context) { * calling recordSearch per row, producing N SP writes on a * potentially-100k-row import. */ - fun recordAllSearches(queries: List) { - if (queries.isEmpty()) return + /** + * Returns the number of fresh queries actually folded into the + * store — same counter pattern as recordAllWatches. + */ + fun recordAllSearches(queries: List): Int { + if (queries.isEmpty()) return 0 val before = _searches.value + val counter = java.util.concurrent.atomic.AtomicInteger(0) val next = _searches.updateAndGet { current -> + counter.set(0) val seen = HashSet(current.size + queries.size) current.forEach { seen.add(it.lowercase()) } val fresh = ArrayList(MAX_SEARCHES) @@ -118,6 +140,7 @@ class HistoryStore(context: Context) { if (q.isEmpty()) continue if (!seen.add(q.lowercase())) continue fresh.add(q) + counter.incrementAndGet() } if (fresh.isEmpty()) return@updateAndGet current (fresh + current).take(MAX_SEARCHES) @@ -125,6 +148,7 @@ class HistoryStore(context: Context) { if (next !== before) { sp.edit().putString(KEY_SEARCHES, json.encodeToString(next)).apply() } + return counter.get() } fun recordSearch(query: String) { diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/data/PlaylistsStore.kt b/strawApp/src/main/kotlin/com/sulkta/straw/data/PlaylistsStore.kt index 57f8979f6..e6cc3aa7a 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/data/PlaylistsStore.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/data/PlaylistsStore.kt @@ -63,6 +63,35 @@ class PlaylistsStore(context: Context) { return pl } + /** + * Bulk-import a playlist with all its items in a single CAS + + * single SP write. SettingsImport's old shape called create() + + * addItem() in a loop — both write SP, and addItem walks every + * playlist linearly per insert. A 100-playlist × 100-items + * NewPipe export was ~10,001 SP commits + ~10M comparisons. + * Round-4 audit HIGH-2. + */ + fun importPlaylist(name: String, items: List): Playlist { + val stampNow = System.currentTimeMillis() + // Dedup within the import + stamp addedAt once. + val seen = HashSet() + val deduped = ArrayList(items.size) + for (it in items) { + if (it.streamUrl.isBlank()) continue + if (!seen.add(it.streamUrl)) continue + deduped.add(it.copy(addedAt = if (it.addedAt == 0L) stampNow else it.addedAt)) + } + val pl = Playlist( + id = UUID.randomUUID().toString(), + name = name.trim().ifBlank { "Untitled" }, + createdAt = stampNow, + items = deduped, + ) + val next = _playlists.updateAndGet { it + pl } + persist(next) + return pl + } + fun delete(id: String) { val next = _playlists.updateAndGet { cur -> cur.filterNot { it.id == id } } persist(next) diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/data/SettingsStore.kt b/strawApp/src/main/kotlin/com/sulkta/straw/data/SettingsStore.kt index 499ace926..f5e1abe5f 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/data/SettingsStore.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/data/SettingsStore.kt @@ -72,18 +72,28 @@ class SettingsStore(context: Context) { sp.edit().putStringSet(KEY_SB_CATS, next.map { it.key }.toSet()).apply() } + // Atomic + idempotent. updateAndGet matches toggle()'s CAS shape; + // idempotency short-circuit means tap-spamming the radio rows + // (or replaying a settings import that doesn't actually change a + // value) doesn't repeatedly hit SP. Round-4 audit MED-2. fun setMaxResolution(r: MaxResolution) { - _maxResolution.value = r + val updated = _maxResolution.updateAndGet { r } == r + if (!updated) return + if (sp.getString(KEY_MAX_RES, null) == r.name) return sp.edit().putString(KEY_MAX_RES, r.name).apply() } fun setThemeMode(t: ThemeMode) { - _themeMode.value = t + val updated = _themeMode.updateAndGet { t } == t + if (!updated) return + if (sp.getString(KEY_THEME, null) == t.name) return sp.edit().putString(KEY_THEME, t.name).apply() } fun setCacheEnabled(enabled: Boolean) { - _cacheEnabled.value = enabled + val before = _cacheEnabled.value + _cacheEnabled.updateAndGet { enabled } + if (before == enabled) return sp.edit().putBoolean(KEY_CACHE_ENABLED, enabled).apply() } 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 9bec03cb2..7e6d5f6d9 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 @@ -12,9 +12,12 @@ package com.sulkta.straw.feature.channel import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import com.sulkta.straw.feature.search.StreamItem +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch data class ChannelUiState( @@ -31,9 +34,19 @@ class ChannelViewModel : ViewModel() { private val _ui = MutableStateFlow(ChannelUiState()) val ui: StateFlow = _ui.asStateFlow() + // Track the active load coroutine — same shape as + // VideoDetailViewModel. Rapid channel switches no longer race; + // the late-arriving older fetch is cancelled. Round-4 audit + // HIGH-2 / MED-1. + private var inFlight: Job? = null + private var loadedUrl: String? = null + fun load(channelUrl: String) { - _ui.value = ChannelUiState(loading = true) - viewModelScope.launch { + if (loadedUrl == channelUrl && _ui.value.videos.isNotEmpty()) return + inFlight?.cancel() + loadedUrl = channelUrl + _ui.update { ChannelUiState(loading = true) } + inFlight = viewModelScope.launch { try { val ch = uniffi.strawcore.channelInfo(channelUrl) val videos = ch.videos.map { v -> @@ -48,25 +61,32 @@ class ChannelViewModel : ViewModel() { uploadDateRelative = v.uploadDateRelative, ) } - _ui.value = ChannelUiState( - loading = false, - name = ch.name, - subscriberCount = ch.subscriberCount, - banner = ch.banner, - avatar = ch.avatar, - videos = videos, - ) + if (loadedUrl != channelUrl) return@launch + _ui.update { + ChannelUiState( + loading = false, + name = ch.name, + subscriberCount = ch.subscriberCount, + banner = ch.banner, + avatar = ch.avatar, + videos = videos, + ) + } } catch (t: Throwable) { - _ui.value = ChannelUiState( - loading = false, - // Scrub before storing — UniFFI/Rust exceptions - // can embed full signed googlevideo URLs in the - // message (NetworkError::Recaptcha { url }). vc=37 - // round-3 audit CVE-1. - error = com.sulkta.straw.util.LogDump.scrubLine( - t.message ?: t.javaClass.simpleName, - ), - ) + if (t is CancellationException) throw t + if (loadedUrl != channelUrl) return@launch + _ui.update { + ChannelUiState( + loading = false, + // Scrub before storing — UniFFI/Rust exceptions + // can embed full signed googlevideo URLs in the + // message (NetworkError::Recaptcha { url }). vc=37 + // round-3 audit CVE-1. + error = com.sulkta.straw.util.LogDump.scrubLine( + t.message ?: t.javaClass.simpleName, + ), + ) + } } } } 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 8ff1c3aea..a54be30a7 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,22 +102,11 @@ 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 - } + // The allowlist itself lives in util.YtUrl now — VideoDetailViewModel + // also gates auto-channelInfo + recordWatch through it. Round-4 + // audit HIGH-4 / HIGH-5. + private fun isAllowedYtUrl(url: String): Boolean = + com.sulkta.straw.util.isAllowedYtUrl(url) suspend fun run(context: Context, zipUri: Uri): Result = withContext(Dispatchers.IO) { @@ -347,11 +336,12 @@ object SettingsImport { } } if (items.isEmpty()) continue - // Use the store's normal create + addItem rather than minting - // a Playlist directly — keeps the atomic-update path - // consistent with user-driven creates. - val created = store.create(name) - for (it in items) store.addItem(created.id, it) + // Bulk import: one CAS + one SP write per playlist + // instead of (1 create + N addItem) writes. Old shape + // produced ~10k SP commits on a 100×100 export, plus + // O(N²) work in addItem's per-call linear scan over + // every playlist. Round-4 audit HIGH-2. + store.importPlaylist(name, items) playlistsAdded++ itemsAdded += items.size } @@ -373,8 +363,8 @@ object SettingsImport { var watchesAvailable = 0 var searchesSeen = 0 var resumePositions = 0 - val searchesBefore = historyStore.searches.value.size - val watchesBefore = historyStore.watches.value.size + var watchesAdded = 0 + var searchesAdded = 0 openDb(dbFile).use { db -> // Search history — feed oldest first so the store ends up with // the most-recent on top after its own dedup + take(MAX). @@ -392,7 +382,7 @@ object SettingsImport { searchesSeen++ } } - historyStore.recordAllSearches(stagedSearches) + searchesAdded = historyStore.recordAllSearches(stagedSearches) // Watch history — newest first via stream_history.access_date, // joined to streams for the metadata we need. @@ -435,7 +425,7 @@ object SettingsImport { watchesSeen++ } } - historyStore.recordAllWatches(staged) + watchesAdded = historyStore.recordAllWatches(staged) // Resume positions — counted, not stored. Future task hooks into // a ResumePositionsStore. @@ -443,11 +433,16 @@ object SettingsImport { if (c.moveToNext()) resumePositions = c.getInt(0) } } - // Report what actually landed in the store after its dedup + caps. + // recordAllWatches / recordAllSearches return the real + // added count (counts fresh videoIds / queries that landed, + // ignoring duplicates and pre-saturated-store truncation). + // Round-4 audit HIGH-7 / MED-2 — previous size_after - + // size_before reported 0 when the store was already at cap + // even when 20 fresh imports actually landed. return HistResult( - watchesAdded = historyStore.watches.value.size - watchesBefore, + watchesAdded = watchesAdded, watchesAvailable = watchesAvailable.takeIf { it > 0 } ?: watchesSeen, - searches = historyStore.searches.value.size - searchesBefore, + searches = searchesAdded, resumePositions = resumePositions, searchesAvailable = searchesSeen, ) 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 602a3fab4..4d524c85a 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 @@ -24,10 +24,14 @@ import com.sulkta.straw.net.RydVotes import com.sulkta.straw.net.SbSegment import com.sulkta.straw.net.SponsorBlockClient import com.sulkta.straw.feature.search.StreamItem +import com.sulkta.straw.util.isAllowedYtUrl +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import kotlinx.coroutines.withContext @@ -85,14 +89,21 @@ class VideoDetailViewModel : ViewModel() { private var loadedUrl: String? = null + // Track the active load coroutine so a rapid tap to a different video + // cancels the prior fetch; otherwise a slow-to-finish older load + // overwrites the newer state and the player ends up streaming A while + // the detail UI shows B. Round-4 audit HIGH-2. + private var inFlight: Job? = null + fun load(streamUrl: String) { // viewModel() is activity-scoped, so the same VM is reused across // navigations. Skip the refetch if the requested URL already has // a resolved state. if (loadedUrl == streamUrl && _ui.value.detail != null) return + inFlight?.cancel() loadedUrl = streamUrl - _ui.value = VideoDetailUiState(loading = true) - viewModelScope.launch { + _ui.update { VideoDetailUiState(loading = true) } + inFlight = viewModelScope.launch { try { // strawcore.streamInfo is suspend on tokio; no Dispatchers.IO wrap. val info = uniffi.strawcore.streamInfo(streamUrl) @@ -104,19 +115,25 @@ class VideoDetailViewModel : ViewModel() { // 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, - ), - ) + // audit Q9. Only record when the resolved URL passes + // the YT allowlist — otherwise extractor-emitted + // non-YT URLs (poisoned related/moreFromChannel) end + // up in Recent Watches and survive process death. + // Round-4 audit HIGH-5. + if (isAllowedYtUrl(streamUrl)) { + withContext(Dispatchers.IO) { + runCatching { + History.get().recordWatch( + WatchHistoryItem( + url = streamUrl, + videoId = videoId, + title = title, + uploader = uploader, + thumbnail = thumb, + watchedAt = 0L, + ), + ) + } } } @@ -144,9 +161,13 @@ class VideoDetailViewModel : ViewModel() { // More from this channel via strawcore.channelInfo — one // Rust round-trip returns the channel's Videos tab pre-mapped. + // Gate the auto-fetch behind the same YT-host allowlist + // we apply to imports: a poisoned uploaderUrl from the + // extractor would otherwise trigger an arbitrary-host + // network call. Round-4 audit HIGH-4. val uploaderUrl = info.uploaderUrl val moreFromChannel: List = - if (uploaderUrl.isNullOrBlank()) emptyList() + if (uploaderUrl.isNullOrBlank() || !isAllowedYtUrl(uploaderUrl)) emptyList() else runCatching { val ch = uniffi.strawcore.channelInfo(uploaderUrl) ch.videos @@ -168,31 +189,43 @@ class VideoDetailViewModel : ViewModel() { val resolved = resolvePlayback(info, segments) - _ui.value = VideoDetailUiState( - loading = false, - detail = VideoDetail( - id = videoId, - title = title, - uploader = uploader, - uploaderUrl = info.uploaderUrl, - viewCount = info.viewCount, - description = info.description, - thumbnail = thumb, - ryd = ryd, - sbSegmentCount = segments.size, - related = related, - moreFromChannel = moreFromChannel, - ), - resolved = resolved, - streamInfo = info, - ) + // Fence the terminal write against late-arriving older + // loads: if a subsequent load(B) cancelled this one but + // we resolved past the suspension point, drop our + // result rather than clobber B's state. Round-4 audit + // HIGH-2. + if (loadedUrl != streamUrl) return@launch + _ui.update { + VideoDetailUiState( + loading = false, + detail = VideoDetail( + id = videoId, + title = title, + uploader = uploader, + uploaderUrl = info.uploaderUrl, + viewCount = info.viewCount, + description = info.description, + thumbnail = thumb, + ryd = ryd, + sbSegmentCount = segments.size, + related = related, + moreFromChannel = moreFromChannel, + ), + resolved = resolved, + streamInfo = info, + ) + } } catch (t: Throwable) { - _ui.value = VideoDetailUiState( - loading = false, - error = com.sulkta.straw.util.LogDump.scrubLine( - t.message ?: t.javaClass.simpleName, - ), - ) + if (t is CancellationException) throw t + if (loadedUrl != streamUrl) return@launch + _ui.update { + VideoDetailUiState( + loading = false, + error = com.sulkta.straw.util.LogDump.scrubLine( + t.message ?: t.javaClass.simpleName, + ), + ) + } } } } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/download/DownloadsScreen.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/download/DownloadsScreen.kt index 30ec8cc94..6689b0508 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/download/DownloadsScreen.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/download/DownloadsScreen.kt @@ -55,7 +55,9 @@ import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.delay +import kotlinx.coroutines.withContext data class DownloadRow( val id: Long, @@ -92,7 +94,11 @@ fun DownloadsScreen() { // animations to update. LaunchedEffect(Unit) { while (true) { - val fresh = queryDownloads(context) + // DownloadManager.query() is a ContentResolver IPC + a + // SQLite cursor walk — disk I/O on the main coroutine + // visibly stutters on devices with hundreds of historical + // downloads. Round-4 audit MED-2. + val fresh = withContext(Dispatchers.IO) { queryDownloads(context) } rows = fresh val active = fresh.any { it.status == DownloadManager.STATUS_RUNNING || 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 5a87e73dd..49da1066c 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 @@ -141,10 +141,12 @@ class SubscriptionFeedViewModel : ViewModel() { val entry = channelCache[ch.url] entry == null || now - entry.fetchedAt >= perChannelTtlMs } - if (anyStale || _ui.value.items.isEmpty()) refresh() + if (anyStale || _ui.value.items.isEmpty()) refreshInternal(force = false) } - fun refresh() { + fun refresh() = refreshInternal(force = true) + + private fun refreshInternal(force: Boolean) { // Cancel any in-flight refresh at the TOP — including before // the empty-channels branch. Without this, a refresh that // ran on a non-empty sub set could still be writing to @@ -168,8 +170,15 @@ class SubscriptionFeedViewModel : ViewModel() { val gate = Semaphore(parallelism) val now = System.currentTimeMillis() coroutineScope { + // force=true (user tapped Refresh): fan out across + // every subscribed channel. force=false (the auto + // refreshIfStale path): only the stale entries. + // Round-4 audit HIGH-8 — previously refresh() also + // filtered to stale-only, so a user-initiated tap + // 5min after the last refresh was a silent no-op. channels .filter { ch -> + if (force) return@filter true val entry = channelCache[ch.url] entry == null || now - entry.fetchedAt >= perChannelTtlMs } 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 bcef3a3a4..3f67ecd75 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 @@ -11,10 +11,13 @@ import com.sulkta.straw.data.FeedCache import com.sulkta.straw.data.History import com.sulkta.straw.data.SearchCache import com.sulkta.straw.data.Settings +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import kotlinx.coroutines.withContext @@ -89,25 +92,33 @@ class SearchViewModel : ViewModel() { runCatching { FeedCache.get().load().values.forEach { addAll(it.items) } } }.distinctBy { it.url } + // Track the active submit so a fresh tap of Search cancels the + // previous network call rather than racing it. Round-4 audit + // HIGH-2: `_ui.value = _ui.value.copy()` patterns + concurrent + // submits were both lost-write hazards. + private var inFlight: Job? = null + fun onQueryChange(q: String) { // 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) + _ui.update { it.copy(query = q, error = null) } if (Settings.get().cacheEnabled.value && q.trim().length >= 2) { val matches = reactiveFilter(q.trim()) if (matches.isNotEmpty()) { - _ui.value = _ui.value.copy( - results = matches, - fromCache = true, - loading = false, - ) + _ui.update { + it.copy( + results = matches, + fromCache = true, + loading = false, + ) + } } else if (_ui.value.fromCache) { - _ui.value = _ui.value.copy(results = emptyList(), fromCache = false) + _ui.update { it.copy(results = emptyList(), fromCache = false) } } } else if (q.isBlank()) { - _ui.value = _ui.value.copy(results = emptyList(), fromCache = false) + _ui.update { it.copy(results = emptyList(), fromCache = false) } } } @@ -126,22 +137,27 @@ class SearchViewModel : ViewModel() { ?.items } else null if (cached != null && cached.isNotEmpty()) { - _ui.value = _ui.value.copy( - loading = true, - error = null, - results = cached, - fromCache = true, - ) + _ui.update { + it.copy( + loading = true, + error = null, + results = cached, + fromCache = true, + ) + } } else { - _ui.value = _ui.value.copy( - loading = true, - error = null, - results = emptyList(), - fromCache = false, - ) + _ui.update { + it.copy( + loading = true, + error = null, + results = emptyList(), + fromCache = false, + ) + } } - viewModelScope.launch { + inFlight?.cancel() + inFlight = viewModelScope.launch { try { // strawcore.search() is suspend on the tokio runtime baked // into libstrawcore.so — no Dispatchers.IO wrap needed. @@ -158,11 +174,17 @@ class SearchViewModel : ViewModel() { uploadDateRelative = r.uploadDateRelative, ) } - _ui.value = _ui.value.copy( - loading = false, - results = items, - fromCache = false, - ) + // Fence terminal write against a fresher submit that + // cancelled this one. Drop our result if the query + // already moved on. + if (_ui.value.query.trim() != q) return@launch + _ui.update { + it.copy( + loading = false, + results = items, + fromCache = false, + ) + } // Record AFTER the search succeeds so mistyped queries // that error out don't pollute the recent-searches list. runCatching { History.get().recordSearch(q) } @@ -176,14 +198,18 @@ class SearchViewModel : ViewModel() { } } } catch (t: Throwable) { + if (t is CancellationException) throw t + if (_ui.value.query.trim() != q) return@launch // Keep the cached preview visible on network failure so // the user still has something to look at while offline. - _ui.value = _ui.value.copy( - loading = false, - error = com.sulkta.straw.util.LogDump.scrubLine( - t.message ?: t.javaClass.simpleName, - ), - ) + _ui.update { + it.copy( + loading = false, + error = com.sulkta.straw.util.LogDump.scrubLine( + t.message ?: t.javaClass.simpleName, + ), + ) + } } } } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/net/Http.kt b/strawApp/src/main/kotlin/com/sulkta/straw/net/Http.kt index a8d629d86..44fb892a4 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/net/Http.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/net/Http.kt @@ -28,19 +28,21 @@ import java.util.concurrent.TimeUnit const val STRAW_USER_AGENT: String = "Mozilla/5.0 (Linux; Android 14) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/124.0.0.0 Mobile Safari/537.36 Straw/1.0" -@Volatile -private var sharedClient: OkHttpClient? = null +// OkHttpClient is internally thread-safe; lazy(SYNCHRONIZED) builds +// exactly once across threads. Round-4 audit MED-6 — the prior +// synchronized(STRAW_USER_AGENT) locked an interned String literal +// shared with any other code in any library that happened to lock +// the same literal. Lazy-delegate avoids the global pool lock. +private val sharedClient: OkHttpClient by lazy(LazyThreadSafetyMode.SYNCHRONIZED) { + OkHttpClient.Builder() + .connectTimeout(15, TimeUnit.SECONDS) + .readTimeout(30, TimeUnit.SECONDS) + .followRedirects(true) + .followSslRedirects(true) + .build() +} -fun strawHttpClient(): OkHttpClient = - sharedClient ?: synchronized(STRAW_USER_AGENT) { - sharedClient ?: OkHttpClient.Builder() - .connectTimeout(15, TimeUnit.SECONDS) - .readTimeout(30, TimeUnit.SECONDS) - .followRedirects(true) - .followSslRedirects(true) - .build() - .also { sharedClient = it } - } +fun strawHttpClient(): OkHttpClient = sharedClient fun ResponseBody.cappedString(maxBytes: Long): String { val cl = contentLength() diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/net/SponsorBlockClient.kt b/strawApp/src/main/kotlin/com/sulkta/straw/net/SponsorBlockClient.kt index 77688c5ed..6055abf12 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/net/SponsorBlockClient.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/net/SponsorBlockClient.kt @@ -12,6 +12,7 @@ import com.sulkta.straw.util.strawLogD import com.sulkta.straw.util.strawLogW import kotlinx.serialization.Serializable import kotlinx.serialization.json.Json +import okhttp3.HttpUrl.Companion.toHttpUrl import okhttp3.Request import java.security.MessageDigest @@ -41,11 +42,19 @@ object SponsorBlockClient { categories: List = listOf("sponsor"), ): List { val prefix = sha256Hex(videoId).substring(0, 4) - val urlStr = "https://sponsor.ajay.app/api/skipSegments/$prefix?" + - "categories=" + buildJsonArray(categories) - strawLogD(TAG) { "fetch: videoId=$videoId prefix=$prefix url=$urlStr" } + // HttpUrl.Builder percent-encodes query values for us. Prior + // string-concat built `?categories=["sponsor","selfpromo"]` + // with literal brackets/quotes — SB happens to accept it + // today, but the next time someone interpolates a non-enum + // string in there it becomes a URL-construction bug. Round-4 + // audit MED-1 / LOW-2. + val url = "https://sponsor.ajay.app/api/skipSegments/$prefix".toHttpUrl() + .newBuilder() + .addQueryParameter("categories", buildJsonArray(categories)) + .build() + strawLogD(TAG) { "fetch: videoId=$videoId prefix=$prefix" } val req = Request.Builder() - .url(urlStr) + .url(url) .header("User-Agent", STRAW_USER_AGENT) .header("Accept", "application/json") .build() diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/util/YtUrl.kt b/strawApp/src/main/kotlin/com/sulkta/straw/util/YtUrl.kt new file mode 100644 index 000000000..b42f1e586 --- /dev/null +++ b/strawApp/src/main/kotlin/com/sulkta/straw/util/YtUrl.kt @@ -0,0 +1,25 @@ +/* + * SPDX-FileCopyrightText: 2026 Sulkta-Coop + * SPDX-License-Identifier: GPL-3.0-or-later + * + * Shared YouTube-host allowlist. Originally lived inside + * SettingsImport for the import-time URL check; round-4 audit + * surfaced two more call sites — VideoDetailViewModel's auto + * channelInfo(uploaderUrl) and recordWatch persistence — that + * needed the same gate. Co-locating the set here so a future + * host (yewtu.be, hypothetical YT mirror) is one edit instead of + * three. + */ + +package com.sulkta.straw.util + +private val ALLOWED_YT_HOSTS: Set = setOf( + "youtube.com", "www.youtube.com", "m.youtube.com", + "music.youtube.com", "youtube-nocookie.com", "www.youtube-nocookie.com", + "youtu.be", +) + +fun isAllowedYtUrl(url: String): Boolean { + val host = runCatching { java.net.URI(url).host?.lowercase() }.getOrNull() ?: return false + return host in ALLOWED_YT_HOSTS +}