From dc9b443777ae10a0410fb1cd929df48a47c00465 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 25 May 2026 22:16:05 +0100 Subject: [PATCH] cleanup --- reviews/CODE-REVIEW.md | 38 ---------- reviews/SUMMARY.md | 165 ----------------------------------------- 2 files changed, 203 deletions(-) delete mode 100644 reviews/CODE-REVIEW.md delete mode 100644 reviews/SUMMARY.md diff --git a/reviews/CODE-REVIEW.md b/reviews/CODE-REVIEW.md deleted file mode 100644 index 2e1e1df..0000000 --- a/reviews/CODE-REVIEW.md +++ /dev/null @@ -1,38 +0,0 @@ -# Code Review — Uncommitted Changes (main branch) - -**Scope:** `desktop/src-tauri/src/main.rs`, `desktop/src-tauri/Cargo.toml`, `desktop/ui/setup.js` -**Date:** 2026-05-25 - ---- - -## Tool Availability - -| Tool | Status | -|------|--------| -| Architecture & Design (manual) | Ran | -| Regression History (`git log`) | Ran | -| Go Static Analysis | N/A — no `.go` files | -| Protobuf Linting | N/A — no `.proto` files | - ---- - -## Consolidated Findings - -| Severity | ID | Finding | Source | Tracked | -|----------|----|---------|--------|---------| -| HIGH | 1 | **`BackendState` two independent `Mutex`es can diverge.** `main.rs:28-31`. `child` and `url` must be co-located in a single `Mutex>` to enforce the both-Some/both-None invariant atomically. Pre-existing. | ARCH-2 | #133 | -| MEDIUM | 2 | **Path resolution duplicated** across `python_path()`, `ffmpeg_path()`, `ffprobe_path()`. `main.rs:~1461–1503`. Env-var override + fallback logic repeated three times. Pre-existing. | DEP-1 | #134 | -| LOW | 3 | ~~**Missing `// SAFETY:` comment on `libc::kill()`.**~~ | ARCH-3 | **FIXED** in this PR | - ---- - -## Regression History - -No regressions detected. All changes are additive. 30-minute timeout preserved in `download_file_blocking`. Prior fix logic in `verify_runtime_pack`, Python validation, and `setup.js` error hints is intact. - ---- - -## Recommended Fix Order - -1. **#1 (HIGH) — BackendState Mutex consolidation.** Tracked in #133. Separate PR; pre-existing design debt. ~1 hour effort. -2. **#2 (MEDIUM) — Path resolver consolidation.** Tracked in #134. Separate refactor PR; pre-existing. ~30 min effort. diff --git a/reviews/SUMMARY.md b/reviews/SUMMARY.md deleted file mode 100644 index bc48c1e..0000000 --- a/reviews/SUMMARY.md +++ /dev/null @@ -1,165 +0,0 @@ -# StemDeck Full Codebase Review - -**Branch:** main | **Version:** v0.5.0-alpha.3 | **Date:** 2026-05-21 - ---- - -## Tool Availability - -| Tool | Status | -|------|--------| -| Static analysis (grep, ast, file reads) | Run across all source files | -| Bandit (Python SAST) | Not run — findings derived from code reading | -| pip-audit | CI-managed; current state documented in `.woodpecker/ci.yml` | -| cargo clippy | Not run locally — findings derived from code reading | -| node --check | Not run — findings derived from code reading | -| pytest | Not run — coverage gaps identified from source only | - ---- - -## System Overview - -**Architecture** - -- FastAPI + uvicorn on 127.0.0.1:PORT (Tauri: dynamic free port; Docker: 8000) -- In-memory job registry (`_jobs: dict[str, Job]`) backed by `registry.json` for terminal jobs only -- Pipeline: asyncio task -> `asyncio.to_thread` -> blocking thread -> `subprocess.Popen` (Demucs) or `subprocess.run` (ffmpeg/ffprobe) -- SSE polling loop: 0.2 s interval, 4-hour deadline — pure poll-and-stream, no push -- Tauri shell: Rust process spawns Python backend; kills on `ExitRequested` / `CloseRequested` -- No authentication, no database — local-only, single-user by design - -**Critical hot paths** - -``` -POST /api/jobs - -> validate input (local) - -> registry check: count queued jobs (lock -> iterate -> release) - -> create_task(run_pipeline) <- returns immediately - -> asyncio.to_thread(_run_blocking) <- blocks thread pool worker - -> YoutubeDL.extract_info x2 (network, serial) - -> subprocess.Popen (Demucs, 5-30 min) <- stderr thread + watchdog thread - -> subprocess.run (ffprobe, ffmpeg x2) <- blocking, up to 300s each - -GET /api/jobs/{id}/stems/{name}.mp3 - -> asyncio.create_subprocess_exec (ffmpeg pipe) <- streaming response - -> yield 64 kB chunks until EOF / disconnect -``` - ---- - -## Security Findings - -| Severity | ID | Finding | STRIDE | OWASP | Tracked | -|----------|----|---------|--------|-------|---------| -| HIGH | S1 | **Stored XSS via track title in innerHTML** — `static/js/catalog.js:851,894`: `track.title` from YouTube metadata injected directly into `innerHTML` without escaping. In Tauri desktop mode, XSS payload can invoke `window.__TAURI__` commands including `open_url`, `store_get`, and `store_set`. | T, E | A03 | — | -| HIGH | S2 | **Stored XSS via channel name in innerHTML** — `static/js/catalog.js:900`: `track.channel` (YouTube uploader field) injected directly into `innerHTML`. Same attack surface as S1. | T | A03 | — | -| HIGH | S3 | **Open URL / scheme injection via `open_url` Tauri command** — `desktop/src-tauri/src/main.rs:933`: caller-supplied URL passed directly to `cmd /c start` (Windows), `open` (macOS), or `xdg-open` (Linux) without scheme validation. No guard against `file:///`, `ms-msdt:`, or other dangerous schemes. Chained with S1/S2 XSS, gives arbitrary app/file launch. | E | A01 | — | -| MEDIUM | S4 | **SSE endpoint missing `JOB_ID_RE` validation** — `app/api/events.py:20`: the only endpoint that does not validate the job ID regex before hitting the registry. All other endpoints validate first. Future code changes inheriting this pattern could be exploitable. | T | A03 | — | -| MEDIUM | S5 | **Stored XSS via YouTube tags in innerHTML** — `static/js/catalog.js:1224`: `tag` strings from `info.get("tags")` lower-cased but not HTML-escaped before `chip.innerHTML = ...`. Lower-casing neutralizes `