Second audit fix sprint — 2 CRIT + 5 HIGH + 5 MED

Second adversarial Opus audit found 2 CRIT + 7 HIGH + 9 MED + 9 LOW
against HEAD 2b948d7. This sprint lands all CRIT, the 5 highest-value
HIGH items, and the most impactful MEDs. Remaining items deferred (see
notes at end).

CRIT-1 (race + torn write on Watch Later / search-history JSON state):
The persistence layer was load->mutate->save with plain open(w). Two
real failure modes were live:
  - Lost-update race: phone fires two RunPlugin wl_add back-to-back,
    each in a fresh Python interpreter. Interpreter A reads [], inserts
    AAA, writes [AAA]; B was already past _load_watch_later (also read
    []) and writes [BBB] — AAA lost.
  - Torn-write loss: open(w) truncates immediately. Kodi crash / Pi
    yank between truncate and json.dump finish leaves a zero-byte or
    partial file. _load* catches JSONDecodeError and returns [],
    silently wiping the user's pinned-videos list on next read.
Fix: _atomic_write_json (mkstemp + fsync + os.replace) + _with_lock
(fcntl.flock on a sibling .lock file). All four persistence functions
(_record_search, _clear_search_history, _add_to_watch_later,
_remove_from_watch_later) refactored to wrap load->mutate->save under
the lock. os.replace is atomic on POSIX same-filesystem; flock is
per-process advisory across the concurrent plugin interpreters.

CRIT-2 (rip allowlist bypass via ..): RIP_DEST_ALLOWLIST check was
string-level starts_with. '/storage/.kodi/temp/../../etc/cron.d' passed
the allowlist but escaped to anywhere. Dormant op (no Python caller
today) but the protocol is a wide-open arbitrary-write primitive
under the sidecar UID (root on LibreELEC). Fix: literal-prefix check
first (cheap reject), then create_dir_all + tokio::fs::canonicalize,
then a second check against the same allowlist on the canonical
result. Defeats .. and symlink escape.

HIGH-1 (SponsorBlock 1 MiB cap was post-hoc): The cap from the first
audit landed AFTER resp.bytes().await — which buffers the entire body
unbounded. A hostile mirror returning multi-GB would OOM the Pi before
the cap fired. Fix: stream via resp.chunk() in a loop, bail as soon as
accumulated bytes > cap. Defends OOM during ingest, not after.

HIGH-2 (Container.Refresh fires in wrong context): _wl_remove_action
unconditionally fired Refresh. Future refactors that expose the Remove
context menu outside the Watch Later listing (or stale context-menu
state across navigation) would refresh the wrong container — search
results would reload pointlessly when the user just hit Remove in WL.
Fix: only Refresh when Container.FolderPath contains
'action=watch_later'.

HIGH-3 (thumbnail shape — promoted from first-audit MED-9): The new
Watch Later code persists rustypipe Player.details which has a
thumbnail string in many versions. _pick_thumbnail called max() on a
string — iterates chars and crashes with AttributeError on the .get
lookup. Live crash-on-render of Watch Later. Fix: _pick_thumbnail now
accepts Any and dispatches on isinstance: empty/None -> '', str ->
str, dict -> .url, list -> filter for dicts then max-by-area.

HIGH-4 (search query unvalidated — length, control chars, surrogates):
Search took an arbitrary string from the addon and shipped it to
rustypipe. Three failure modes:
  - 10 MB query allocates 10 MB on stdin pipe + another 10 MB in
    serde_json::from_str + materializes in rustypipe's HTTP call. ~30
    MB blip per request on the 1 GB Pi.
  - Newlines in query produce multi-line entries in
    tracing::info!(query, ...) — log injection (mild on this stack,
    but obfuscates real entries).
  - Lone UTF-16 surrogates aren't a crash but produce a confusing
    error path.
Fix: validate_query() at the sidecar dispatch — 2 KB length cap,
reject control chars (TAB allowed since YouTube treats it as
whitespace). Addon-side _record_search now also collapses whitespace
via ' '.join(query.split()).

MED-1 (clear_history flicker): _clear_history_action finalized with
succeeded=True THEN Container.Update — caused a half-tick of empty
directory before the nav replaced it. Fix: succeeded=False first,
then the replace navigates cleanly.

MED-3 (LibreELEC-only fallback): _addon_data_path's xbmcvfs fallback
hardcoded /storage/.kodi/userdata/... — non-existent on Linux desktop
Kodi etc. Fix: fall back to ~/.kodi/userdata/addon_data/... which is
portable. Tests + dev rigs now persist correctly.

MED-4 (Response::ok clobber): Added debug_assert! so any future op
returning its own 'ok' key is caught in debug builds. Not a fix
per se — but a tripwire.

MED-5 (codec regex single-quote): Made _MIME_CODEC_RE accept either
quote style. Belt-and-braces against upstream MIME format drift.

MED-8 (SponsorBlockMonitor orphaned across plugin boundary): When we
delegate to plugin.video.youtube, our monitor runs in our plugin's
context against xbmc.Player()'s global state. If the user starts a
different video mid-monitor, the segments are for the old one — would
spuriously skip the new content. Fix: capture player.getPlayingFile()
at start, bail if it changes mid-loop.

MED-9 / LOW-2 carried over from first audit and tracked here too.

Smoke-verified via JSON-RPC (browse-only, no playback, someone still
watching the TV):
- 3000-byte query rejected: 'query too long: 3000 bytes (cap 2048)'
- newline-in-query rejected: 'query contains control characters'
- legit search returns expected results
- wl_add + watch_later directory renders without thumbnail crash

Remaining deferred (cosmetic / no current impact):
- MED-2 (WL staleness): accept for v0.1, add lazy refresh later
- MED-6 (byte-length validate_youtube_id error message): cosmetic
- MED-7 (_lan_ip multi-NIC): not currently a problem on the family
  LAN, document and revisit if Tailscale lands on the Pi
- LOW-1..9: defensive polish, no real risk
- First-audit's deferred MED/LOWs: still defer-able

Addon v0.0.16.

sidecar/crates/torttube-sidecar/src/main.rs

@@ -104,8 +104,15 @@ enum ErrorKind {
 impl Response {
     fn ok(value: serde_json::Value) -> Self {
         // Inject ok:true into the value if it's an object, otherwise wrap it.
+        // Sanity-check: no op should ever return its own `ok` field — the
+        // wire protocol reserves `ok` for our wrapper. Catches drift if any
+        // future op forwards a remote response that happens to carry `ok`.
         let value = match value {
             serde_json::Value::Object(mut map) => {
+                debug_assert!(
+                    !map.contains_key("ok"),
+                    "op-handler returned a value with its own `ok` key; would be clobbered"
+                );
                 map.insert("ok".into(), serde_json::Value::Bool(true));
                 serde_json::Value::Object(map)
             }
@@ -194,12 +201,41 @@ async fn handle_line(line: &str) -> Response {
             if let Err(e) = validate_youtube_id(&id) {
                 return Response::err(ErrorKind::BadRequest, e);
             }
+            // Resolve the path against the literal allowlist FIRST (cheap reject),
+            // then canonicalize after create_dir_all to defeat `..` traversal.
+            // String-level starts_with passes `/storage/.kodi/temp/../../etc` — we
+            // need a real filesystem-aware check. Audit CRIT-2 + HIGH-5 (2nd pass).
             if !RIP_DEST_ALLOWLIST.iter().any(|p| dest_dir.starts_with(p)) {
                 return Response::err(
                     ErrorKind::BadRequest,
                     format!("rip dest_dir not in allowlist: {dest_dir}"),
                 );
             }
+            if let Err(e) = tokio::fs::create_dir_all(&dest_dir).await {
+                return Response::err(
+                    ErrorKind::Io,
+                    format!("create_dir_all {dest_dir}: {e}"),
+                );
+            }
+            let canonical = match tokio::fs::canonicalize(&dest_dir).await {
+                Ok(p) => p,
+                Err(e) => {
+                    return Response::err(
+                        ErrorKind::Io,
+                        format!("canonicalize {dest_dir}: {e}"),
+                    )
+                }
+            };
+            let canonical_str = canonical.to_string_lossy();
+            let inside_allowlist = RIP_DEST_ALLOWLIST.iter().any(|p| {
+                canonical_str.starts_with(p.trim_end_matches('/'))
+            });
+            if !inside_allowlist {
+                return Response::err(
+                    ErrorKind::BadRequest,
+                    format!("rip dest_dir escapes allowlist after canonicalize: {canonical_str}"),
+                );
+            }
             match rip::rip(&id, &dest_dir).await {
                 Ok(v) => Response::ok(v),
                 Err(e) => e.into(),
@@ -215,6 +251,9 @@ async fn handle_line(line: &str) -> Response {
             }
         }
         Request::Search { query, limit } => {
+            if let Err(e) = validate_query(&query) {
+                return Response::err(ErrorKind::BadRequest, e);
+            }
             match resolve::search(&query, limit.min(MAX_LIMIT)).await {
                 Ok(v) => Response::ok(v),
                 Err(e) => e.into(),
@@ -236,6 +275,32 @@ async fn handle_line(line: &str) -> Response {
     }
 }
 
+/// Validate a free-form search query. Audit HIGH-4 (2nd pass).
+/// - Length cap (2KB) — prevents a multi-megabyte query from sucking up RAM
+///   on the Pi 4 + provoking the OOM killer.
+/// - No control chars (except TAB which YouTube treats as whitespace).
+///   Protects against log-injection via newlines (sidecar emits a single
+///   `tracing::info!(query, ...)` per call; embedded newlines look like
+///   separate log records). Also protects against null bytes that would
+///   break OS-level args if we ever stop using stdin.
+fn validate_query(q: &str) -> Result<(), String> {
+    const MAX_QUERY_BYTES: usize = 2048;
+    if q.is_empty() {
+        return Err("query is empty".into());
+    }
+    if q.len() > MAX_QUERY_BYTES {
+        return Err(format!(
+            "query too long: {} bytes (cap {})",
+            q.len(),
+            MAX_QUERY_BYTES
+        ));
+    }
+    if q.chars().any(|c| c.is_control() && c != '\t') {
+        return Err("query contains control characters".into());
+    }
+    Ok(())
+}
+
 /// Strict YouTube video-id shape check — 11 chars from [A-Za-z0-9_-].
 /// Centralized so every op that takes an `id` enforces the same contract;
 /// returns a clear error message for `BadRequest` rather than passing a junk

sidecar/crates/torttube-sidecar/src/sponsor.rs

@@ -54,7 +54,7 @@ pub(crate) async fn fetch(id: &str, categories: &[String]) -> anyhow::Result<ser
         .timeout(std::time::Duration::from_secs(10))
         .build()?;
 
-    let resp = client.get(&url).send().await?;
+    let mut resp = client.get(&url).send().await?;
     if resp.status() == reqwest::StatusCode::NOT_FOUND {
         // No segments matching the prefix at all.
         return Ok(serde_json::json!({ "segments": [] }));
@@ -63,18 +63,20 @@ pub(crate) async fn fetch(id: &str, categories: &[String]) -> anyhow::Result<ser
         anyhow::bail!("sponsorblock http {}", resp.status());
     }
 
-    // Cap the response body at 1 MiB. A normal prefix collision returns
-    // tens of KB; a degenerate response (or a hostile API mirror) returning
-    // gigabytes would otherwise be deserialized straight into memory before
-    // we filter to our target video_id.
-    const SPONSORBLOCK_MAX_BYTES: usize = 1 * 1024 * 1024;
-    let bytes = resp.bytes().await?;
-    if bytes.len() > SPONSORBLOCK_MAX_BYTES {
-        anyhow::bail!(
-            "sponsorblock response too large: {} bytes (cap {})",
-            bytes.len(),
-            SPONSORBLOCK_MAX_BYTES
-        );
+    // Stream the body and bail as soon as accumulated bytes exceed the cap —
+    // do NOT buffer the entire response first, otherwise a hostile mirror or
+    // compromised DNS returning a multi-GB response would OOM the sidecar
+    // before any cap check fires. Audit HIGH-1 (2nd pass).
+    const SPONSORBLOCK_MAX_BYTES: usize = 1024 * 1024;
+    let mut bytes: Vec<u8> = Vec::with_capacity(64 * 1024);
+    while let Some(chunk) = resp.chunk().await? {
+        if bytes.len() + chunk.len() > SPONSORBLOCK_MAX_BYTES {
+            anyhow::bail!(
+                "sponsorblock response too large (>{} bytes)",
+                SPONSORBLOCK_MAX_BYTES
+            );
+        }
+        bytes.extend_from_slice(&chunk);
     }
     let body: Vec<ApiResponse> = serde_json::from_slice(&bytes)?;