Sulkta-Coop/straw
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

vc=38: round-3 audit-fix sprint — 9 HIGH + 7 MED

Browse source 
Three round-3 Opus audits ran on vc=37. NO new CRITs (round-2 work
held) but real new HIGHs — several were vc=37 own-goals.

HIGH
  R3-1  recordAllWatches dropped import on capacity=0. Old: when
        watches store hit MAX_WATCHES (50), capacity=0, the whole
        import was discarded silently. New: build fresh import list
        capped at MAX_WATCHES, then combine + take(MAX_WATCHES) so
        imports always land (truncating oldest current entries).
        Also: skip SP write when next === before (no-op import on
        already-saturated store no longer thrashes disk).
        New recordAllSearches with same shape — round-3 CVE MED-6:
        importHistory was per-row recordSearch.
  R3-2  / CVE-2  SubscriptionsStore.addAll counter race. The vc=36
        size-delta fix snapshot `cur = _subs.value` BEFORE
        updateAndGet, so a concurrent toggle inflated `added`. New:
        AtomicInteger reset at the start of each lambda re-run,
        counted by checking each ref against the pre-image inside
        the CAS. Exactly the additions THIS call made.
  R3-3  refresh() empty-channels didn't cancel inFlight. Cancel
        moved to the top of refresh() unconditionally so a refresh
        on the prior sub set is killed before the empty branch
        clears + wipes disk.
        clearInMemoryCache also cancels inFlight — without it, a
        cache-disable flip during a refresh could see fetchChannelInto
        re-populate the just-cleared map.
  R3-4  Non-atomic `_ui.value = it.copy(...)` at init hydrate path
        and clearInMemoryCache. Replaced with `_ui.update {}` for
        atomicity vs concurrent refresh writes. init's
        lastFetchedAt write now uses maxOf so it never regresses
        past a fresh refresh value.
  CVE-1 state.error rendered raw UniFFI/Rust error strings to UI
        — NetworkError::Recaptcha { url } embeds full signed
        googlevideo URL. User screenshots a "reCAPTCHA at <URL>"
        banner → leak. All four VMs (Channel/Detail/Feed/Search)
        now scrub via LogDump.scrubLine before storing.
  CVE-3 pruneCacheToSubs in init can clobber concurrent
        fetchChannelInto writes. init's putAll → putIfAbsent so
        a fresh entry from a parallel refresh isn't overwritten
        with disk-stale data.
  CVE-4 SIGNED_PARAM_RE over-redacted short tokens (`\bn=`
        matched `n=42` counters from any wrapped lib). Split into
        SIGNED_PARAM_LONG_RE (signature/sparams/lsig/cpn/expire/
        pot/sig/key — match anywhere) and SIGNED_PARAM_SHORT_RE
        (n/mn/ms/mo/pl/ip/ei — require `[?&]` immediately before).
  Func-HIGH-1 refresh() swallowed CancellationException as a
        user-visible error. Spam-tapping Refresh produced a
        "refresh failed: StandaloneCoroutineCancelled" banner.
        Re-throw CancellationException; catch only real errors.

MED
  R3-5  reactiveFilter did N `.lowercase()` allocations per
        keystroke. Switched to contains(ignoreCase = true) — zero
        allocations.
  CVE-MED-5  FileProvider cache-path was "." (whole cacheDir,
        including SettingsImport workdirs). Narrowed to "logs/";
        LogDump.capture now writes to cacheDir/logs/ to match.
  CVE-MED-7  Downloader.Request.setTitle was the raw title
        (bidi-override / control chars possible). Switched to
        safeTitle.
  CVE-MED-8 Rust hello_from_rust value-log scrubbed to name_len.
  Func-LOW-4 recordAllWatches skip-write-on-no-change (`next !==
        before`).

Deferred to a follow-up (not user-facing this ship):
  R3-MED-6 — Settings setMaxResolution/setThemeMode/setCacheEnabled
        not atomic via updateAndGet. Inconsistent with toggle()
        but the Switch UI throttles enough that no real race.
  R3-MED-8 — Minibar play-button reads live controller.isPlaying
        instead of listener-tracked. One-frame oscillation on
        super-fast double-tap.
  R3-LOW — collectAsState vs collectAsStateWithLifecycle drift.
  Func-LOW-6 — refreshIfStale isActive check is TOCTOU on a
        non-existent multi-threaded call surface (LaunchedEffect
        + button are both Main).
This commit is contained in:
Kayos 2026-05-25 14:29:32 -07:00
parent 780bb6152c
commit cbdba302ce
12 changed files with 167 additions and 61 deletions
Download patch file Download diff file Expand all files Collapse all files

64
strawApp/src/main/kotlin/com/sulkta/straw/data/HistoryStore.kt
View file

@ -61,36 +61,70 @@ class HistoryStore(context: Context) {
* 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.
* Walks input newest-first (input is fed oldest-first), filters
* blanks + already-seen videoIds, prepends to current, then takes
* MAX_WATCHES. Imports WIN over older current entries when the
* store is at the cap — the vc=37 first cut silently discarded
* the whole import in that case (round-3 audit HIGH-1).
*
* 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.
* Skips the SP write when the resulting list is identical (by
* 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<WatchHistoryItem>) {
if (items.isEmpty()) return
val before = _watches.value
val next = _watches.updateAndGet { current ->
val seen = HashSet<String>(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<WatchHistoryItem>(capacity)
// Walk newest-first; stop as soon as we have capacity.
// Build the import list newest-first. Capped at
// MAX_WATCHES on its own so we don't over-allocate
// even on a 50k-row hostile export.
val fresh = ArrayList<WatchHistoryItem>(MAX_WATCHES)
val it = items.listIterator(items.size)
while (it.hasPrevious() && fresh.size < capacity) {
while (it.hasPrevious() && fresh.size < MAX_WATCHES) {
val item = it.previous()
if (item.videoId.isBlank()) continue
if (!seen.add(item.videoId)) continue
fresh.add(item)
}
if (fresh.isEmpty()) return@updateAndGet current
// Combine + cap. take() truncates older `current` entries
// when we'd exceed MAX_WATCHES, so imports always land.
(fresh + current).take(MAX_WATCHES)
}
sp.edit().putString(KEY_WATCHES, json.encodeToString(next)).apply()
if (next !== before) {
sp.edit().putString(KEY_WATCHES, json.encodeToString(next)).apply()
}
}
/**
* Bulk import for search history. Same pattern as
* recordAllWatches — single SP write regardless of input size.
* vc=37 round-3 audit CVE-MED-6: SettingsImport.importHistory was
* calling recordSearch per row, producing N SP writes on a
* potentially-100k-row import.
*/
fun recordAllSearches(queries: List<String>) {
if (queries.isEmpty()) return
val before = _searches.value
val next = _searches.updateAndGet { current ->
val seen = HashSet<String>(current.size + queries.size)
current.forEach { seen.add(it.lowercase()) }
val fresh = ArrayList<String>(MAX_SEARCHES)
val it = queries.listIterator(queries.size)
while (it.hasPrevious() && fresh.size < MAX_SEARCHES) {
val q = it.previous().trim()
if (q.isEmpty()) continue
if (!seen.add(q.lowercase())) continue
fresh.add(q)
}
if (fresh.isEmpty()) return@updateAndGet current
(fresh + current).take(MAX_SEARCHES)
}
if (next !== before) {
sp.edit().putString(KEY_SEARCHES, json.encodeToString(next)).apply()
}
}
fun recordSearch(query: String) {

21
strawApp/src/main/kotlin/com/sulkta/straw/data/SubscriptionsStore.kt
View file

@ -73,22 +73,27 @@ class SubscriptionsStore(context: Context) {
* caller can report an "added X" stat.
*/
fun addAll(refs: List<ChannelRef>): Int {
// 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
// Count NEW refs by checking each input URL against the
// current state's pre-image inside the CAS lambda. Captures
// exactly the additions this call made — concurrent
// toggle()s that race the CAS don't inflate the count (vc=37
// round-3 audit HIGH-2/CVE-2). The counter lives in an
// AtomicInteger so each lambda re-run resets it correctly.
val counter = java.util.concurrent.atomic.AtomicInteger(0)
val next = _subs.updateAndGet { state ->
counter.set(0)
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
if (r.url !in byUrl) {
byUrl[r.url] = r
counter.incrementAndGet()
}
}
byUrl.values.toList()
}
persist(next)
return next.size - cur.size
return counter.get()
}
fun clear() {

8
strawApp/src/main/kotlin/com/sulkta/straw/feature/channel/ChannelViewModel.kt
View file

@ -59,7 +59,13 @@ class ChannelViewModel : ViewModel() {
} catch (t: Throwable) {
_ui.value = ChannelUiState(
loading = false,
error = t.message ?: t.javaClass.simpleName,
// 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,
),
)
}
}

9
strawApp/src/main/kotlin/com/sulkta/straw/feature/dataimport/SettingsImport.kt
View file

@ -378,16 +378,21 @@ object SettingsImport {
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).
// Stage + bulk-write — vc=37 round-3 audit CVE MED-6:
// per-row recordSearch was N SP writes on potentially
// 100k+ rows. The SELECT also lacked a LIMIT; added now.
val stagedSearches = mutableListOf<String>()
db.rawQuery(
"SELECT search FROM search_history WHERE service_id=? ORDER BY creation_date ASC",
"SELECT search FROM search_history WHERE service_id=? ORDER BY creation_date ASC LIMIT 50000",
arrayOf(YT_SERVICE_ID.toString()),
).use { c ->
while (c.moveToNext()) {
val q = c.getString(0) ?: continue
historyStore.recordSearch(q)
stagedSearches += q
searchesSeen++
}
}
historyStore.recordAllSearches(stagedSearches)
// Watch history — newest first via stream_history.access_date,
// joined to streams for the metadata we need.

4
strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt
View file

@ -189,7 +189,9 @@ class VideoDetailViewModel : ViewModel() {
} catch (t: Throwable) {
_ui.value = VideoDetailUiState(
loading = false,
error = t.message ?: t.javaClass.simpleName,
error = com.sulkta.straw.util.LogDump.scrubLine(
t.message ?: t.javaClass.simpleName,
),
)
}
}

6
strawApp/src/main/kotlin/com/sulkta/straw/feature/download/Downloader.kt
View file

@ -65,7 +65,11 @@ object Downloader {
// returned below, so user-facing UX is unaffected.
val req = runCatching {
DownloadManager.Request(Uri.parse(url))
.setTitle(title)
// Sanitized title — bidi-overrides and control chars
// in extractor output would otherwise render in
// DownloadsScreen's row title. vc=37 round-3 audit
// CVE MED-7.
.setTitle(safeTitle)
.setDescription("Straw — ${kind.name.lowercase()}")
.setNotificationVisibility(DownloadManager.Request.VISIBILITY_HIDDEN)
.setVisibleInDownloadsUi(false)

55
strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/SubscriptionFeedViewModel.kt
View file

@ -25,6 +25,7 @@ import com.sulkta.straw.data.Settings
import com.sulkta.straw.data.Subscriptions
import com.sulkta.straw.feature.search.StreamItem
import com.sulkta.straw.util.strawLogW
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.async
@ -78,14 +79,24 @@ class SubscriptionFeedViewModel : ViewModel() {
if (!Settings.get().cacheEnabled.value) return@launch
val saved = withContext(Dispatchers.IO) { FeedCache.get().load() }
if (saved.isEmpty()) return@launch
channelCache.putAll(saved)
// putIfAbsent (not putAll) — refresh() may have started
// populating fresh entries during our IO suspension; we
// must not overwrite those with disk-stale values.
// vc=37 round-3 audit CVE-3.
saved.forEach { (url, entry) -> channelCache.putIfAbsent(url, entry) }
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,
)
val savedTs = saved.values.maxOfOrNull { it.fetchedAt } ?: 0L
// _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),
lastFetchedAt = maxOf(it.lastFetchedAt, savedTs),
)
}
}
}
}
@ -134,21 +145,23 @@ class SubscriptionFeedViewModel : ViewModel() {
}
fun refresh() {
// 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
// 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.
inFlight?.cancel()
val channels = Subscriptions.get().subs.value
if (channels.isEmpty()) {
_ui.update { SubscriptionFeedUiState(loading = false, items = emptyList()) }
_ui.update { it.copy(loading = false, items = emptyList(), error = null) }
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()
_ui.update { it.copy(loading = true, error = null) }
inFlight = viewModelScope.launch {
try {
@ -181,10 +194,18 @@ class SubscriptionFeedViewModel : ViewModel() {
}
}
} catch (t: Throwable) {
// Re-throw cancellation so spam-tapping Refresh (or
// toggling cache OFF→ON during a refresh) doesn't
// surface a "refresh failed: StandaloneCoroutineCancelled"
// banner above the cached items. vc=37 round-3 audit
// function-correctness HIGH-1.
if (t is CancellationException) throw t
_ui.update {
it.copy(
loading = false,
error = t.message ?: t.javaClass.simpleName,
error = com.sulkta.straw.util.LogDump.scrubLine(
t.message ?: t.javaClass.simpleName,
),
)
}
}
@ -264,8 +285,14 @@ class SubscriptionFeedViewModel : ViewModel() {
* stayed visible until process death. vc=35 audit MED-C13.
*/
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.
inFlight?.cancel()
channelCache.clear()
_ui.value = _ui.value.copy(items = emptyList(), lastFetchedAt = 0L)
// Use _ui.update for atomicity vs concurrent refresh writes
// (round-3 audit HIGH-4).
_ui.update { it.copy(items = emptyList(), lastFetchedAt = 0L) }
}
}

12
strawApp/src/main/kotlin/com/sulkta/straw/feature/search/SearchViewModel.kt
View file

@ -180,7 +180,9 @@ class SearchViewModel : ViewModel() {
// the user still has something to look at while offline.
_ui.value = _ui.value.copy(
loading = false,
error = t.message ?: t.javaClass.simpleName,
error = com.sulkta.straw.util.LogDump.scrubLine(
t.message ?: t.javaClass.simpleName,
),
)
}
}
@ -193,11 +195,13 @@ class SearchViewModel : ViewModel() {
* after each successful submit and at VM construction.
*/
private fun reactiveFilter(q: String): List<StreamItem> {
val needle = q.lowercase()
// contains(ignoreCase=true) on the raw fields avoids the
// 3N+ String allocations per keystroke that `.lowercase()`
// copy-and-compare produced. Round-3 audit MED-5.
return pool.asSequence()
.filter { item ->
item.title.lowercase().contains(needle)
|| item.uploader.lowercase().contains(needle)
item.title.contains(q, ignoreCase = true)
|| item.uploader.contains(q, ignoreCase = true)
}
.take(60)
.toList()

31
strawApp/src/main/kotlin/com/sulkta/straw/util/LogDump.kt
View file

@ -43,12 +43,16 @@ object LogDump {
runCatching {
val pid = Process.myPid()
val timestamp = SimpleDateFormat("yyyyMMdd-HHmmss", Locale.US).format(Date())
val outFile = File(context.cacheDir, "straw-logs-$timestamp.txt")
val tmpFile = File(context.cacheDir, "straw-logs-$timestamp.txt.tmp")
// Write to cacheDir/logs/ — vc=37 round-3 audit CVE MED-5
// narrowed the FileProvider scope from the whole cacheDir
// to just this subdir, so dumps must land here.
val logsDir = File(context.cacheDir, "logs").apply { mkdirs() }
val outFile = File(logsDir, "straw-logs-$timestamp.txt")
val tmpFile = File(logsDir, "straw-logs-$timestamp.txt.tmp")
// Sweep old dumps before writing the new one so cacheDir
// doesn't grow per export.
context.cacheDir.listFiles { _, name ->
logsDir.listFiles { _, name ->
name.startsWith("straw-logs-") && (name.endsWith(".txt") || name.endsWith(".tmp"))
}?.forEach { it.delete() }
@ -107,18 +111,27 @@ object LogDump {
var s = line
// Pre-signed googlevideo URLs: keep host visible, drop path+query.
s = GOOGLEVIDEO_URL_RE.replace(s, "https://<host>.googlevideo.com/<scrubbed>")
// 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=<scrubbed>")
// Long, distinctive token names — match anywhere.
s = SIGNED_PARAM_LONG_RE.replace(s, "$1=<scrubbed>")
// Short single-letter / two-letter tokens — require `[?&]`
// immediately before to avoid eating innocent counters.
s = SIGNED_PARAM_SHORT_RE.replace(s, "$1$2=<scrubbed>")
return s
}
private val GOOGLEVIDEO_URL_RE = Regex(
"""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|n|lsig|ei|key|sparams)=([^&\s"']+)""",
// Long tokens are unique enough to match anywhere. Short tokens
// (n, mn, ms, mo, pl, ip, ei) require `[?&]` immediately before
// so we don't redact innocuous `n=42` counters from other libs.
// vc=37 round-3 audit CVE-4.
private val SIGNED_PARAM_LONG_RE = Regex(
"""\b(signature|sparams|lsig|cpn|expire|pot|sig|key)=([^&\s"']+)""",
RegexOption.IGNORE_CASE,
)
private val SIGNED_PARAM_SHORT_RE = Regex(
"""([?&])(n|mn|ms|mo|pl|ip|ei)=([^&\s"']+)""",
RegexOption.IGNORE_CASE,
)
}

9
strawApp/src/main/res/xml/file_paths.xml
View file

@ -1,8 +1,11 @@
<?xml version="1.0" encoding="utf-8"?>
<paths xmlns:android="http://schemas.android.com/apk/res/android">
<!-- LogDump shares logcat captures to a chooser-picked app. Limited
to cacheDir so a pasted URI can't grant access to user files. -->
<cache-path name="logs" path="." />
<!-- LogDump shares logcat captures to a chooser-picked app.
Narrowed to a `logs/` subdir of cacheDir (was the entire
cacheDir) so a future bug that builds an attacker-influenced
FileProvider URI can't reach SettingsImport workdirs or
other cache state. vc=37 round-3 audit CVE MED-5. -->
<cache-path name="logs" path="logs/" />
<!-- Completed downloads. Downloader uses
setDestinationInExternalFilesDir(DIRECTORY_MOVIES + "/audio" |