From c960a1f424ea044e5f32b09e388f49eaf1b4e46b Mon Sep 17 00:00:00 2001 From: Kayos Date: Tue, 26 May 2026 20:53:25 -0700 Subject: [PATCH] vc=68: audit-fix sprint round 1 (11 HIGH + MED batch) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Block B — enrichment lifecycle drift: * SubscriptionFeedViewModel tracks enrichJob, cancelled in refresh + clearInMemoryCache so spam-refresh and cache-toggle no longer leave a globalScope coroutine writing to a destroyed _ui * Enrich now runs on viewModelScope, channels snapshotted at job start so the terminal merge doesn't read a stale subs list * mergeFromCache moved off Main on both the refresh path AND the init-hydration path — 750-item flatMap+sort+regex no longer blocks the UI thread * VideoDetailViewModel dual loadedUrl bookkeeping collapsed to the UiState field only; the rejected-URL path also stamps loadedUrl so the gate reads coherently Block A — auto-update authenticity: * AppUpdateClient pins the fdroid.sulkta.com leaf SPKI + the Let's Encrypt E7 intermediate via OkHttp CertificatePinner * file.name accepted only when matching ^/[A-Za-z0-9._-]+\.apk$ * versionCode clamped to (0, 10_000_000] before we trust the 'update available' notification — a hostile index can no longer pin us to MAX_VALUE Block C — captureResumePosition perf: * ResumePositionsStore.record short-circuits when the existing entry matches position+duration so the 5s poll's before !== next guard actually skips the SP write * JSON encode + SP write off Main via globalScope IO Block D — Rust feed.rs hardening: * Shared reqwest Client via OnceLock — 50 channels no longer pay 50 TLS handshakes * Response body capped at 2 MiB via bytes_stream — adversarial feeds can't OOM the JVM * parse_rss returns partial results on quick-xml errors instead of nuking everything already parsed * extract_channel_id widened (m./www./http(s)?/trailing path) and validates exact 24-char UC<22 base64-ish> * Skip entries with empty title/published * iso_to_relative future dates → 'just now' (clock skew no longer pins items to top) * civil_to_days year clamp 1970..=2200 before the i64 arithmetic * Redirect chain capped at 3 * Dropped the broken lexicographic sort on upload_date_relative * Cap parsed entries at 50 per channel MED batch: * ThumbnailProgressOverlay uses derivedStateOf so only rows whose specific entry changed recompose on the 5s positions tick * EnrichmentStore.put short-circuits on identical view+duration so re-enrich within TTL doesn't write SP * EnrichmentStore.load prunes TTL-expired entries on hydration * FeedRefreshWorker distinguishes transient (Result.retry) from parse (Result.success) failures * WorkManager interval coerceAtLeast(15L) on both schedulers --- buildSrc/src/main/kotlin/ProjectConfig.kt | 4 +- rust/strawcore/src/feed.rs | 197 ++++++++++++++---- .../com/sulkta/straw/data/EnrichmentStore.kt | 20 +- .../sulkta/straw/data/ResumePositionsStore.kt | 42 +++- .../feature/detail/VideoDetailViewModel.kt | 28 ++- .../feature/feed/FeedRefreshScheduler.kt | 4 +- .../straw/feature/feed/FeedRefreshWorker.kt | 18 +- .../feature/feed/SubscriptionFeedViewModel.kt | 66 ++++-- .../straw/feature/player/ThumbnailProgress.kt | 20 +- .../straw/feature/update/AppUpdateClient.kt | 63 +++++- .../straw/feature/update/UpdateScheduler.kt | 6 +- 11 files changed, 385 insertions(+), 83 deletions(-) diff --git a/buildSrc/src/main/kotlin/ProjectConfig.kt b/buildSrc/src/main/kotlin/ProjectConfig.kt index 5e3d33135..1b4ad26a7 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 = 67 -const val STRAW_VERSION_NAME = "0.1.0-CA" +const val STRAW_VERSION_CODE = 68 +const val STRAW_VERSION_NAME = "0.1.0-CB" const val STRAW_APPLICATION_ID = "com.sulkta.straw" diff --git a/rust/strawcore/src/feed.rs b/rust/strawcore/src/feed.rs index 259dee238..395b66840 100644 --- a/rust/strawcore/src/feed.rs +++ b/rust/strawcore/src/feed.rs @@ -13,6 +13,7 @@ // the full stream_info path to fetch the rich metadata when actually // needed. +use std::sync::OnceLock; use std::time::Duration; use futures::stream::{self, StreamExt}; @@ -24,6 +25,23 @@ use crate::search::SearchItem; const RSS_BASE: &str = "https://www.youtube.com/feeds/videos.xml?channel_id="; const MAX_CONCURRENT: usize = 50; const PER_CHANNEL_TIMEOUT_S: u64 = 8; +/// Cap on the body bytes we'll read for a single RSS fetch. Real YT +/// Atom feeds are ~5-30 KB; 2 MiB leaves comfortable headroom while +/// blocking a hostile or compromised host from streaming GB-scale +/// bodies into JVM memory inside the 8s timeout. Round-67 audit +/// rust-HIGH-5. +const RSS_MAX_BYTES: usize = 2 * 1024 * 1024; +/// Cap on parsed entries per channel — RSS normally returns 15. +/// 50 leaves headroom for one-off legitimate variance; anything +/// past that is a sign the feed isn't what we expect. +/// Round-67 audit rust-MED-6. +const RSS_MAX_ENTRIES: usize = 50; +/// Year range we trust civil-to-days math for. Strawcore RSS only +/// emits real-world recent uploads; clamping here turns adversarial +/// year fields into a parse failure rather than i64 overflow. +/// Round-67 audit rust-CRIT-1. +const YEAR_MIN: i32 = 1970; +const YEAR_MAX: i32 = 2200; /// Hybrid-backfill metadata: just the two fields RSS doesn't return /// (view count + duration). Kotlin calls this lazily for visible feed @@ -55,6 +73,28 @@ pub async fn enrich_feed_item( }) } +/// Shared reqwest Client — DNS resolver + TLS keepalive + connection +/// pool live here so a 50-channel fan-out reuses one pool instead of +/// paying 50 handshakes. Round-67 audit rust-HIGH-4. +static RSS_CLIENT: OnceLock = OnceLock::new(); + +fn rss_client() -> Result<&'static Client, StrawcoreError> { + if let Some(c) = RSS_CLIENT.get() { + return Ok(c); + } + let client = Client::builder() + .timeout(Duration::from_secs(PER_CHANNEL_TIMEOUT_S)) + .user_agent(concat!("Mozilla/5.0 (Android; Mobile; Straw/", env!("CARGO_PKG_VERSION"), ")")) + // Cap redirect chains so a misconfigured/hostile feed can't + // spin a server out of our 8s budget. Round-67 audit rust-LOW-8. + .redirect(reqwest::redirect::Policy::limited(3)) + .build() + .map_err(|e| StrawcoreError::Extractor { + msg: format!("http client build: {e}"), + })?; + Ok(RSS_CLIENT.get_or_init(|| client)) +} + /// Single-channel RSS — Kotlin keeps its per-channel cache + fan-out /// (parallelism cranked to 50 in the wrapper). Each call is ~50-150ms /// instead of the ~500ms channelInfo page-scrape, so a 50-sub refresh @@ -65,14 +105,8 @@ pub async fn channel_feed_rss( ) -> Result, StrawcoreError> { crate::runtime::ensure_initialized(); log::info!("strawcore::channel_feed_rss url_len={}", channel_url.len()); - let client = Client::builder() - .timeout(Duration::from_secs(PER_CHANNEL_TIMEOUT_S)) - .user_agent("Mozilla/5.0 (Android; Mobile; Straw/0.1)") - .build() - .map_err(|e| StrawcoreError::Extractor { - msg: format!("http client build: {e}"), - })?; - Ok(fetch_channel_rss(&client, &channel_url).await.unwrap_or_default()) + let client = rss_client()?; + Ok(fetch_channel_rss(client, &channel_url).await.unwrap_or_default()) } /// Bulk subscription feed fan-out — for callers that want one round-trip @@ -88,68 +122,109 @@ pub async fn subscription_feed( if channel_urls.is_empty() { return Ok(Vec::new()); } - let client = Client::builder() - .timeout(Duration::from_secs(PER_CHANNEL_TIMEOUT_S)) - .user_agent("Mozilla/5.0 (Android; Mobile; Straw/0.1)") - .build() - .map_err(|e| StrawcoreError::Extractor { - msg: format!("http client build: {e}"), - })?; + let client = rss_client()?; let results: Vec> = stream::iter(channel_urls.into_iter()) - .map(|url| { - let client = client.clone(); - async move { fetch_channel_rss(&client, &url).await.unwrap_or_default() } - }) + .map(|url| async move { fetch_channel_rss(client, &url).await.unwrap_or_default() }) .buffer_unordered(MAX_CONCURRENT) .collect() .await; - let mut flat: Vec = results.into_iter().flatten().collect(); - // Newest first by published timestamp baked into the upload_date_relative - // field at parse time — RSS already returns entries newest-first per - // channel so we mostly just need cross-channel interleave. - flat.sort_by(|a, b| b.upload_date_relative.cmp(&a.upload_date_relative)); - Ok(flat) + // Per-channel ordering is RSS-served-newest-first. Cross-channel + // interleave is the caller's responsibility — Kotlin's mergeFromCache + // sorts by parsed recency, which is the source of truth. Returning + // the flat list as-is. (vc=66 prior code sorted lexicographically + // on the relative-date STRING, which is wrong because "10 hours + // ago" < "2 hours ago" in cmp order — round-67 audit rust-HIGH-6.) + Ok(results.into_iter().flatten().collect()) } async fn fetch_channel_rss(client: &Client, channel_url: &str) -> Option> { let channel_id = extract_channel_id(channel_url)?; let url = format!("{RSS_BASE}{channel_id}"); - let body = client + let resp = client .get(&url) .send() .await .ok()? .error_for_status() - .ok()? - .text() - .await .ok()?; + // Streaming body read with a hard byte cap — `.text()` reads + // unbounded into a String. Round-67 audit rust-HIGH-5. + let body = read_capped_body(resp).await?; parse_rss(&body, channel_id) } -/// Extract the `UCxxx` channel ID from a channel URL. Handles the -/// common shapes: +/// Drain a reqwest Response into a String, bailing out (return None) if +/// the body exceeds RSS_MAX_BYTES. Round-67 audit rust-HIGH-5. +async fn read_capped_body(resp: reqwest::Response) -> Option { + use futures::StreamExt; + let mut total = 0usize; + let mut buf: Vec = Vec::with_capacity(32 * 1024); + let mut stream = resp.bytes_stream(); + while let Some(chunk_result) = stream.next().await { + let chunk = chunk_result.ok()?; + total = total.saturating_add(chunk.len()); + if total > RSS_MAX_BYTES { + log::warn!("strawcore::rss body exceeded {RSS_MAX_BYTES} bytes; aborting"); + return None; + } + buf.extend_from_slice(&chunk); + } + String::from_utf8(buf).ok() +} + +/// Extract the `UCxxx` channel ID from a channel URL. Accepts the +/// shapes the Android app actually has in Subscriptions plus the ones +/// users paste from share intents: /// * `https://www.youtube.com/channel/UCxxx...` -/// * `https://www.youtube.com/UCxxx...` (canonical clone) +/// * `https://youtube.com/channel/UCxxx...` +/// * `http(s)://m.youtube.com/channel/UCxxx...` +/// * trailing `/videos`, `?si=...`, etc — anything after the ID is dropped /// * raw `UCxxx...` (already an ID) /// +/// Real YT channel IDs are EXACTLY 24 chars (`UC` + 22 base64-ish). +/// Round-67 audit rust-HIGH-1. +/// /// `@handle` URLs are NOT supported here — RSS requires the channel ID. -/// Callers that only have an @handle should resolve via channel_info() -/// once, cache the ID into Subscriptions, and pass the ID forever after. +/// Callers with @handles should resolve via channel_info() once and +/// cache the ID into Subscriptions. fn extract_channel_id(input: &str) -> Option { let trimmed = input.trim(); - if let Some(stripped) = trimmed.strip_prefix("https://www.youtube.com/channel/") { - return Some(stripped.split('/').next()?.to_string()); + let trimmed_lower = trimmed.to_lowercase(); + // Match the ":///channel/" prefix in a single sweep + // so we accept http/https + www./m. variants without four-way + // string-strip ladders. + const PREFIXES: &[&str] = &[ + "https://www.youtube.com/channel/", + "https://youtube.com/channel/", + "https://m.youtube.com/channel/", + "http://www.youtube.com/channel/", + "http://youtube.com/channel/", + "http://m.youtube.com/channel/", + ]; + for p in PREFIXES { + if let Some(idx) = trimmed_lower.find(p) { + let rest = &trimmed[idx + p.len()..]; + let id = rest.split(|c: char| c == '/' || c == '?' || c == '#').next()?; + return validate_channel_id(id); + } } - if let Some(stripped) = trimmed.strip_prefix("https://youtube.com/channel/") { - return Some(stripped.split('/').next()?.to_string()); + validate_channel_id(trimmed) +} + +/// A real YouTube channel ID is `UC` followed by exactly 22 chars from +/// `[A-Za-z0-9_-]`. Round-67 audit rust-HIGH-1. +fn validate_channel_id(id: &str) -> Option { + if id.len() != 24 || !id.starts_with("UC") { + return None; } - if trimmed.starts_with("UC") && trimmed.len() >= 22 && trimmed.len() <= 26 { - return Some(trimmed.to_string()); + if !id.bytes().skip(2).all(|b| { + matches!(b, b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'_' | b'-') + }) { + return None; } - None + Some(id.to_string()) } fn parse_rss(body: &str, channel_id: String) -> Option> { @@ -242,7 +317,11 @@ fn parse_rss(body: &str, channel_id: String) -> Option> { let name = e.name(); let local = local_name(name.as_ref()); if local == "entry" { - if !video_id.is_empty() { + // Skip entries missing the load-bearing fields — + // an empty title renders as a blank card the user + // can't tap, and an empty published collapses the + // recency sort. Round-67 audit rust-HIGH-2. + if !video_id.is_empty() && !title.is_empty() && !published.is_empty() { items.push(SearchItem { url: format!("https://www.youtube.com/watch?v={video_id}"), title: title.clone(), @@ -266,6 +345,12 @@ fn parse_rss(body: &str, channel_id: String) -> Option> { // first in the fan-out. Caught 2026-05-26. upload_date_relative: iso_to_relative(&published), }); + if items.len() >= RSS_MAX_ENTRIES { + // Defense-in-depth against a feed that + // ships thousands of blocks. + // Round-67 audit rust-MED-6. + return Some(items); + } } in_entry = false; depth = 0; @@ -275,7 +360,15 @@ fn parse_rss(body: &str, channel_id: String) -> Option> { text_target = None; } Ok(Event::Eof) => break, - Err(_) => return None, + // Partial-parse on error: return whatever we've already + // collected rather than throwing the whole batch away. + // A truncated body (EOF mid-stream on a flaky network) + // would otherwise silently disappear the channel. + // Round-67 audit rust-CRIT-3. + Err(e) => { + log::warn!("strawcore::rss parse error after {} items: {e}", items.len()); + return Some(items); + } _ => {} } buf.clear(); @@ -307,7 +400,16 @@ fn iso_to_relative(iso: &str) -> String { .duration_since(std::time::UNIX_EPOCH) .map(|d| d.as_secs() as i64) .unwrap_or(0); - format_relative(now_secs.saturating_sub(secs)) + // A device with a skewed clock can see RSS timestamps as future- + // dated. saturating_sub returns 0 → "0 seconds ago" → sorts to + // top, which is the LTT/WTYP-recurrence vector. Treat future + // dates as "just now" so the relative-string sort behaves and + // a single skewed item doesn't pin itself at the top of the + // feed. Round-67 audit rust-HIGH-7. + if secs > now_secs { + return "just now".to_string(); + } + format_relative(now_secs - secs) } fn parse_rfc3339_secs(s: &str) -> Option { @@ -327,6 +429,13 @@ fn parse_rfc3339_secs(s: &str) -> Option { let hh: u32 = time_parts.next()?.parse().ok()?; let mm: u32 = time_parts.next()?.parse().ok()?; let ss: u32 = time_parts.next()?.parse().ok()?; + // Year clamp BEFORE civil_to_days — out-of-range years overflow + // the era arithmetic in debug, wrap in release. A hostile feed + // serving year=2147483647 must not produce junk timestamps. + // Round-67 audit rust-CRIT-1. + if !(YEAR_MIN..=YEAR_MAX).contains(&y) { + return None; + } if !(1..=12).contains(&m) || !(1..=31).contains(&d) || hh > 23 || mm > 59 || ss > 60 { return None; } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/data/EnrichmentStore.kt b/strawApp/src/main/kotlin/com/sulkta/straw/data/EnrichmentStore.kt index c18448bfd..5851fda9c 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/data/EnrichmentStore.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/data/EnrichmentStore.kt @@ -76,6 +76,17 @@ class EnrichmentStore(context: Context) { ) val before = _entries.value val next = _entries.updateAndGet { current -> + // Round-67 audit HIGH-4: short-circuit when the cached + // value is already the same view+duration — re-enriching + // within TTL otherwise allocates a new Map every call + // and the `before !== next` guard never triggers, so a + // refresh-after-refresh hammers the SP file. + val existing = current[videoId] + if (existing != null && + existing.viewCount == entry.viewCount && + existing.durationSeconds == entry.durationSeconds) { + return@updateAndGet current + } val withEntry = current + (videoId to entry) if (withEntry.size > MAX_ENRICHMENTS) { withEntry.entries @@ -98,7 +109,14 @@ class EnrichmentStore(context: Context) { private fun load(): Map = runCatching { val s = sp.getString(KEY, null) ?: return emptyMap() - json.decodeFromString>(s) + val loaded = json.decodeFromString>(s) + // Round-67 audit MED-6: prune TTL-expired entries on load + // so the store doesn't accumulate dead weight up to + // MAX_ENRICHMENTS over time. `Forever` TTL skips the prune. + val ttl = Settings.get().cacheTtl.value + if (ttl.isForever) return loaded + val cutoff = System.currentTimeMillis() - ttl.ms + loaded.filterValues { it.fetchedAt >= cutoff } }.getOrDefault(emptyMap()) } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/data/ResumePositionsStore.kt b/strawApp/src/main/kotlin/com/sulkta/straw/data/ResumePositionsStore.kt index 42ddfc8db..9ef0c07bc 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/data/ResumePositionsStore.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/data/ResumePositionsStore.kt @@ -17,10 +17,13 @@ package com.sulkta.straw.data import android.content.Context import android.content.SharedPreferences +import com.sulkta.straw.StrawApp +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.updateAndGet +import kotlinx.coroutines.launch import kotlinx.serialization.Serializable import kotlinx.serialization.json.Json @@ -94,7 +97,27 @@ class ResumePositionsStore(context: Context) { ) val before = _positions.value val next = _positions.updateAndGet { current -> + // Round-67 audit HIGH-6: short-circuit value-equality — + // a 5s poll tick that finds the same (position, duration, + // wall-time) for an existing entry returns `current` + // unchanged so the outer `next !== before` guard + // actually short-circuits the SP write. + // + // lastWatchedAt updates every tick by definition, but + // ResumePosition equality on position+duration alone is + // ALL we care about for "did anything meaningful change." + // We re-stamp lastWatchedAt only when the player position + // actually advances. + val existing = current[videoId] + if (existing != null && + existing.positionMs == entry.positionMs && + existing.durationMs == entry.durationMs) { + return@updateAndGet current + } val withEntry = current + (videoId to entry) + // Skip sort+associate when we're under the cap (the + // common case at default 500). Sort is O(n log n); + // associate allocates another map. Round-67 audit HIGH-6. if (withEntry.size > maxResumes()) { // Drop oldest by lastWatchedAt — newcomers always land // because the entry we just added is by definition the @@ -108,7 +131,13 @@ class ResumePositionsStore(context: Context) { } } if (next !== before) { - sp.edit().putString(KEY_POSITIONS, json.encodeToString(next)).apply() + // JSON encode + SP write off Main — encoding 100k entries + // would be ~50-100 ms on a low-end device, and the 5s + // captureResumePosition poll runs on Main. Round-67 + // audit HIGH-6. + StrawApp.globalScope.launch(Dispatchers.IO) { + sp.edit().putString(KEY_POSITIONS, json.encodeToString(next)).apply() + } } } @@ -125,13 +154,20 @@ class ResumePositionsStore(context: Context) { if (videoId !in current) current else current - videoId } if (next !== before) { - sp.edit().putString(KEY_POSITIONS, json.encodeToString(next)).apply() + StrawApp.globalScope.launch(Dispatchers.IO) { + sp.edit().putString(KEY_POSITIONS, json.encodeToString(next)).apply() + } } } fun clearAll() { + val before = _positions.value _positions.updateAndGet { emptyMap() } - sp.edit().putString(KEY_POSITIONS, json.encodeToString(emptyMap())).apply() + if (before.isNotEmpty()) { + StrawApp.globalScope.launch(Dispatchers.IO) { + sp.edit().putString(KEY_POSITIONS, json.encodeToString(emptyMap())).apply() + } + } } private fun load(): Map = runCatching { 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 76a12d77e..665e57afe 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 @@ -109,8 +109,6 @@ class VideoDetailViewModel : ViewModel() { private val _ui = MutableStateFlow(VideoDetailUiState()) val ui: StateFlow = _ui.asStateFlow() - 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 @@ -120,23 +118,31 @@ class VideoDetailViewModel : ViewModel() { 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 + // a resolved state. Snapshot _ui once so the two reads agree. + val snap = _ui.value + if (snap.loadedUrl == streamUrl && snap.detail != null) return // Same YT-host gate as ChannelViewModel — covers the case // where a tap on a poisoned related-card lands here. // Round-5 audit MED-3. Round-6 audit HIGH-1: cancel any // in-flight load on rejection too — otherwise the // late-arriving prior-job's fence still PASSES (loadedUrl // wasn't moved) and clobbers the "Unsupported URL" error - // banner. + // banner. round-67 audit HIGH-7: also set loadedUrl on this + // path so the gate reads coherently for any caller that + // checks _ui.value.loadedUrl on the rejected path. if (!isAllowedYtUrl(streamUrl)) { inFlight?.cancel() inFlight = null - _ui.update { VideoDetailUiState(loading = false, error = "Unsupported URL") } + _ui.update { + VideoDetailUiState( + loading = false, + error = "Unsupported URL", + loadedUrl = streamUrl, + ) + } return } inFlight?.cancel() - loadedUrl = streamUrl _ui.update { VideoDetailUiState(loading = true, loadedUrl = streamUrl) } inFlight = viewModelScope.launch { try { @@ -261,8 +267,10 @@ class VideoDetailViewModel : ViewModel() { // 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 + // HIGH-2. Round-67 audit HIGH-7: single source of + // truth — read loadedUrl from _ui rather than a + // shadowing field. + if (_ui.value.loadedUrl != streamUrl) return@launch _ui.update { VideoDetailUiState( loading = false, @@ -288,7 +296,7 @@ class VideoDetailViewModel : ViewModel() { } } catch (t: Throwable) { if (t is CancellationException) throw t - if (loadedUrl != streamUrl) return@launch + if (_ui.value.loadedUrl != streamUrl) return@launch _ui.update { VideoDetailUiState( loading = false, diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/FeedRefreshScheduler.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/FeedRefreshScheduler.kt index 9d0624845..ad4f8922f 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/FeedRefreshScheduler.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/FeedRefreshScheduler.kt @@ -29,8 +29,10 @@ object FeedRefreshScheduler { wm.cancelUniqueWork(WORK_NAME) return } + // WorkManager 15-minute periodic floor — see UpdateScheduler. + // Round-67 audit MED-4. val request = PeriodicWorkRequestBuilder( - s.bgFeedRefreshInterval.value.minutes, + s.bgFeedRefreshInterval.value.minutes.coerceAtLeast(15L), TimeUnit.MINUTES, ).setConstraints( Constraints.Builder() diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/FeedRefreshWorker.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/FeedRefreshWorker.kt index 312732be1..849988922 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/FeedRefreshWorker.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/FeedRefreshWorker.kt @@ -27,6 +27,8 @@ import com.sulkta.straw.data.Settings import com.sulkta.straw.data.Subscriptions import com.sulkta.straw.feature.search.StreamItem import com.sulkta.straw.util.strawLogI +import com.sulkta.straw.util.strawLogW +import java.io.IOException class FeedRefreshWorker( context: Context, @@ -41,9 +43,21 @@ class FeedRefreshWorker( // One bulk call via the Rust subscriptionFeed fan-out. Returns // a flat list; we group by uploaderUrl to rebuild the per- // channel cache shape FeedCacheStore expects. - val flat = runCatching { + // + // Round-67 audit MED-5: distinguish transient failures + // (network down, timeout) from parse failures. The former + // wants Result.retry() so WorkManager re-attempts within the + // current window with exponential backoff; without this, a + // 30-second offline blip eats a full 6-hour refresh cycle. + val flat = try { uniffi.strawcore.subscriptionFeed(subs.map { it.url }) - }.getOrNull() ?: return Result.success() + } catch (e: IOException) { + strawLogW("FeedRefresh") { "transient network failure, retrying: ${e.message}" } + return Result.retry() + } catch (e: Throwable) { + strawLogW("FeedRefresh") { "non-transient failure, giving up this cycle: ${e.message}" } + return Result.success() + } val now = System.currentTimeMillis() val grouped: Map = flat 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 eb393c851..391105a11 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 @@ -91,12 +91,17 @@ class SubscriptionFeedViewModel : ViewModel() { if (channels.isNotEmpty()) { pruneCacheToSubs(channels) val savedTs = saved.values.maxOfOrNull { it.fetchedAt } ?: 0L + // Compute the merge off-Main first (round-67 audit + // HIGH-1) — flatMap + regex + sort on hydration was + // running on Main and could add ~10-20 ms to cold + // start on a slow phone. + val hydrated = withContext(Dispatchers.Default) { mergeFromCache(channels) } // _ui.update so a concurrent refresh()'s state write // doesn't race with this copy. vc=37 round-3 audit // HIGH-4. Only advance lastFetchedAt — never regress. _ui.update { it.copy( - items = mergeFromCache(channels), + items = hydrated, lastFetchedAt = maxOf(it.lastFetchedAt, savedTs), ) } @@ -134,6 +139,16 @@ class SubscriptionFeedViewModel : ViewModel() { /** Live refresh job, so spam-tapping Refresh doesn't fan out racing fetches. */ private var inFlight: Job? = null + /** + * The background enrichment job runs on StrawApp.globalScope so it + * outlives the VM's viewModelScope — but a refresh-cancel must + * still kill the *previous* enrichment so we don't pile up + * overlapping fan-outs (8-wide × N overlapping refreshes blows the + * concurrency budget). Tracked here, cancelled in the same places + * `inFlight` is. Round-67 audit HIGH-2/3/8. + */ + private var enrichJob: 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 @@ -160,8 +175,11 @@ class SubscriptionFeedViewModel : ViewModel() { // channelCache when the user unsubscribes from the last // channel; we'd clear() then immediately repopulate with // phantom entries when the prior fetchChannelInto resolved. - // vc=37 round-3 audit HIGH-3. + // vc=37 round-3 audit HIGH-3. Also kill any in-flight + // enrichment fan-out so we don't end up with N overlapping + // enrich jobs piling up under spam-refresh — round-67 HIGH-8. inFlight?.cancel() + enrichJob?.cancel() val channels = Subscriptions.get().subs.value if (channels.isEmpty()) { _ui.update { it.copy(loading = false, items = emptyList(), error = null) } @@ -193,7 +211,11 @@ class SubscriptionFeedViewModel : ViewModel() { .awaitAll() } pruneCacheToSubs(channels) - val freshItems = mergeFromCache(channels) + // Move flatMap + per-item regex + sort off Main — + // viewModelScope.launch runs on Main by default and + // mergeFromCache is non-trivial on a 500-item merge. + // Round-67 audit HIGH-1. + val freshItems = withContext(Dispatchers.Default) { mergeFromCache(channels) } _ui.update { SubscriptionFeedUiState( loading = false, @@ -205,7 +227,11 @@ class SubscriptionFeedViewModel : ViewModel() { // viewCount=0 + durationSeconds=0; kick a bounded // background job that calls enrichFeedItem for the // top items and pumps a fresh _ui emit when done. - enrichVisibleItems(freshItems) + // Pass the channels snapshot so the enrich job's + // terminal mergeFromCache uses what was current at + // job start, not whatever the user's subs are by + // the time enrichment finishes ~2s later. + enrichVisibleItems(freshItems, channels) // Persist what we just freshened. Off the main thread — // JSON encode on 30 subs * 30 items is small but not // free, and SharedPreferences.apply is async anyway. @@ -317,15 +343,19 @@ class SubscriptionFeedViewModel : ViewModel() { * complete in ~2s. Skipped per-item when FeedEnrichment already * has a fresh hit (TTL controlled by Settings.cacheTtl). * - * Runs OFF viewModelScope so a refresh-cancel doesn't kill an - * enrichment that's almost done — the background fill is for - * NEXT-open paint, no rush. Uses StrawApp.globalScope. + * Runs on viewModelScope (round-67 audit HIGH-2): outliving the VM + * would mean a destroyed _ui can still receive a stale emit (and + * mergeFromCache reads a now-cleared channelCache). The next + * VM instance does its own enrichment on next refresh; nothing + * is lost by not finishing the prior one. Tracked in enrichJob so + * refresh + clearInMemoryCache can cancel it. */ - private fun enrichVisibleItems(items: List) { + private fun enrichVisibleItems(items: List, channelsSnapshot: List) { val take = items.take(ENRICH_HEAD_COUNT) .filter { it.viewCount <= 0L && it.durationSeconds <= 0L } if (take.isEmpty()) return - com.sulkta.straw.StrawApp.globalScope.launch { + enrichJob?.cancel() + enrichJob = viewModelScope.launch { val gate = Semaphore(ENRICH_PARALLELISM) coroutineScope { take.map { item -> @@ -348,11 +378,14 @@ class SubscriptionFeedViewModel : ViewModel() { } }.awaitAll() } - // Pump a fresh emit so the UI picks up the overlay. - withContext(Dispatchers.Main) { - val channels = Subscriptions.get().subs.value - _ui.update { it.copy(items = mergeFromCache(channels)) } + // Compute the merge off-Main — flatMap + per-item regex + // + sort over up to 500 items is too much for the UI + // thread. Then hop to Main only for the StateFlow emit. + // Round-67 audit HIGH-1. + val merged = withContext(Dispatchers.Default) { + mergeFromCache(channelsSnapshot) } + _ui.update { it.copy(items = merged) } } } @@ -385,8 +418,13 @@ class SubscriptionFeedViewModel : ViewModel() { fun clearInMemoryCache() { // Cancel any in-flight refresh — without this, fetchChannelInto // coroutines mid-execution would re-populate the cache after - // the clear. Round-3 audit function MED-3. + // the clear. Round-3 audit function MED-3. Also cancel any + // enrichment fan-out (lives on globalScope, NOT viewModelScope) + // — otherwise a still-running enrichment would write to + // FeedEnrichment + then push a merged emit reading the empty + // channelCache. Round-67 audit HIGH-3. inFlight?.cancel() + enrichJob?.cancel() channelCache.clear() // Use _ui.update for atomicity vs concurrent refresh writes // (round-3 audit HIGH-4). diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ThumbnailProgress.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ThumbnailProgress.kt index 26daa53f0..4e31e6b12 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ThumbnailProgress.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ThumbnailProgress.kt @@ -26,7 +26,9 @@ import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.runtime.collectAsState +import androidx.compose.runtime.derivedStateOf import androidx.compose.runtime.getValue +import androidx.compose.runtime.remember import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.clip @@ -57,10 +59,20 @@ fun BoxScope.ThumbnailProgressOverlay(videoId: String?) { // scroll jank (vc=67). The Lifecycle pause optimization doesn't // matter for a foreground feed that's only collected while the // composable is on screen anyway. - val positions by Resume.get().positions.collectAsState() - val entry = positions[videoId] ?: return - if (entry.durationMs <= 0L) return - val fraction = (entry.positionMs.toFloat() / entry.durationMs.toFloat()) + // + // Round-67 audit MED-2: derivedStateOf isolates each row's + // dependency to ONLY its own videoId's entry. Without this, the + // 5s captureResumePosition tick re-emits the entire positions + // map → every visible thumbnail recomposes. With it, only rows + // whose specific entry changed recompose. + val positionsFlow = Resume.get().positions + val positions by positionsFlow.collectAsState() + val entry by remember(videoId) { + derivedStateOf { positions[videoId] } + } + val resolved = entry ?: return + if (resolved.durationMs <= 0L) return + val fraction = (resolved.positionMs.toFloat() / resolved.durationMs.toFloat()) .coerceIn(0f, 1f) Box( modifier = Modifier diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/update/AppUpdateClient.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/update/AppUpdateClient.kt index 4f0eacc6c..69428cec1 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/update/AppUpdateClient.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/update/AppUpdateClient.kt @@ -16,17 +16,37 @@ package com.sulkta.straw.feature.update import com.sulkta.straw.BuildConfig import com.sulkta.straw.util.runCatchingCancellable +import com.sulkta.straw.util.strawLogW import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import kotlinx.serialization.Serializable import kotlinx.serialization.json.Json +import okhttp3.CertificatePinner import okhttp3.OkHttpClient import okhttp3.Request import java.util.concurrent.TimeUnit +private const val INDEX_HOST = "fdroid.sulkta.com" private const val INDEX_URL = "https://fdroid.sulkta.com/fdroid/repo/index-v2.json" private const val REPO_BASE = "https://fdroid.sulkta.com/fdroid/repo" +/** + * Only accept file names that look like a plain APK basename. The index + * controls a string we substitute into an `ACTION_VIEW` intent; without + * sanitization a hostile or compromised index could ship `..//host/x.apk` + * or worse. Round-67 audit HIGH-5. + */ +private val APK_NAME_RE = Regex("""^/[A-Za-z0-9._-]+\.apk$""") + +/** + * Sanity-cap on parsed versionCode. Straw vc is currently low double + * digits; ten million is a horizon we won't hit organically but blocks + * a hostile index from latching us to Long.MAX_VALUE and burying every + * legitimate update behind a "you're already up to date" check. + * Round-67 audit HIGH-5. + */ +private const val MAX_PLAUSIBLE_VC = 10_000_000L + data class UpdateInfo( val versionCode: Long, val versionName: String, @@ -34,9 +54,31 @@ data class UpdateInfo( ) object AppUpdateClient { + /** + * Pin two Subject-Public-Key-Info SHA-256 hashes against + * fdroid.sulkta.com so an off-tree CA misissue can't ship the + * user an attacker-signed index. Round-67 audit HIGH-5. + * + * - sha256/8ofd... — current leaf SPKI. Rotates every ~90 days + * with each Let's Encrypt renewal; an app update before the + * next rotation refreshes this pin. + * - sha256/y7xV... — Let's Encrypt E7 intermediate SPKI. Stable + * for years; serves as the rotation-safety pin while we push + * a new leaf hash. + * + * When the leaf pin no longer matches (post-rotation), OkHttp + * still accepts the chain because the E7 intermediate pin + * matches. The next app release rolls the leaf forward. + */ + private val pinner: CertificatePinner = CertificatePinner.Builder() + .add(INDEX_HOST, "sha256/8ofdiPS6TAiUx9zb2O7Qa9IKZQ3D2i+18teKCrz/MqA=") + .add(INDEX_HOST, "sha256/y7xVm0TVJNahMr2sZydE2jQH8SquXV9yLF9seROHHHU=") + .build() + private val http: OkHttpClient = OkHttpClient.Builder() .connectTimeout(15, TimeUnit.SECONDS) .readTimeout(15, TimeUnit.SECONDS) + .certificatePinner(pinner) .build() private val json = Json { ignoreUnknownKeys = true } @@ -58,10 +100,29 @@ object AppUpdateClient { val best = pkg.versions.values .maxByOrNull { it.manifest.versionCode } ?: return@runCatchingCancellable null + // Reject implausible versionCodes outright — see + // MAX_PLAUSIBLE_VC. Round-67 audit HIGH-5. + if (best.manifest.versionCode <= 0 || + best.manifest.versionCode > MAX_PLAUSIBLE_VC) { + strawLogW("StrawUpdate") { + "rejecting implausible versionCode=${best.manifest.versionCode}" + } + return@runCatchingCancellable null + } + // Strict APK-basename match before we hand this off to + // ACTION_VIEW. Anything else gets logged + dropped. + // Round-67 audit HIGH-5. + val fileName = best.file.name + if (!APK_NAME_RE.matches(fileName)) { + strawLogW("StrawUpdate") { + "rejecting unsafe file.name=${fileName.take(80)}" + } + return@runCatchingCancellable null + } UpdateInfo( versionCode = best.manifest.versionCode, versionName = best.manifest.versionName.orEmpty(), - apkUrl = "$REPO_BASE${best.file.name}", + apkUrl = "$REPO_BASE$fileName", ) }.getOrNull() } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/update/UpdateScheduler.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/update/UpdateScheduler.kt index b24ffe6e5..d91cf3461 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/update/UpdateScheduler.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/update/UpdateScheduler.kt @@ -34,8 +34,12 @@ object UpdateScheduler { wm.cancelUniqueWork(WORK_NAME) return } + // WorkManager floors periodic intervals at 15 minutes. + // coerceAtLeast(15) future-proofs against a smaller enum case + // landing without anyone noticing the silent clamp. Round-67 + // audit MED-4. val request = PeriodicWorkRequestBuilder( - interval.minutes, + interval.minutes.coerceAtLeast(15L), TimeUnit.MINUTES, ).setConstraints( Constraints.Builder()