From a6a8c5f57843f29f64acf2e4aa61d3a36f9d2544 Mon Sep 17 00:00:00 2001 From: debont80 Date: Mon, 29 Jun 2026 13:47:45 -0400 Subject: [PATCH] Update CODE-AUDIT.md: living checklist with stable IDs Reformat from a one-off security report into a living checklist. Adds the 2026-06-29 architectural review findings and a second polish pass; all original security findings marked completed. Items carry stable IDs for tracking across sessions. Co-Authored-By: Claude Sonnet 4.6 --- CODE-AUDIT.md | 1636 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 1350 insertions(+), 286 deletions(-) diff --git a/CODE-AUDIT.md b/CODE-AUDIT.md index 43bd463..1402e43 100644 --- a/CODE-AUDIT.md +++ b/CODE-AUDIT.md @@ -1,342 +1,1406 @@ -# AeroFetch Code Audit +# Code Audit -**Date:** 2026-06-23 -**Scope:** Full security & correctness review of main process, preload, IPC contract, and renderer stores -**Overall:** Strong security fundamentals (context isolation, sandboxing, argument-injection defense); findings are refinements, not foundational gaps +Living checklist of audit findings for AeroFetch. Security & correctness were reviewed +2026-06-23 (all fixed — see Completed). The architectural review (2026-06-29) added the +structural items; a second polish pass (2026-06-29) added the smaller inconsistencies. +Items carry stable IDs so we can check them off as they land this session. + +Severity = structural leverage / risk of future bugs, not "app is broken" (it isn't). --- -## Executive Summary +# 1.0 Release-Readiness Audit (lead-engineer synthesis) -AeroFetch's security posture is notably thoughtful: +*A pre-release synthesis of the ~385 catalogued findings below — root causes, a release gate, and the +consequential issues per dimension (severity · reasoning · fix · benefit). The detailed, ID'd backlog +follows. No rewrite required; everything here is incremental.* -- **Layered argument-injection defense**: `assertHttpUrl` validators + `--` terminator before the URL, preventing yt-dlp flag injection -- **Tight process model**: `contextIsolation: true`, `sandbox: true`, `nodeIntegration: false`; thin preload forwarding only typed IPC -- **File opening safety**: [reveal.ts](src/main/reveal.ts) allowlist restricts to media extensions, blocks `.exe`/`.bat`/`.ps1` -- **Settings validation**: Type-checked before persistence; `spawn` used without `shell: true` -- **Good test coverage** of risky logic (especially [buildArgs.test.ts](test/buildArgs.test.ts)) +## Verdict -The findings below are opportunities to tighten existing defenses and fix performance gaps. None are foundational breaks — with one exception added later: **C1**, a missing bundled `ffprobe.exe` that broke duration-dependent post-processing, surfaced by the real-download smoke test and now fixed. +**Not 1.0-ready as-is, but close — roughly 3–4 focused PRs away.** The foundation is genuinely strong: +disciplined security model (context isolation, sandbox, argv-injection defense, host-pinned signed-checksum +updater), a clean pure/impure architecture, and good unit coverage of the pure logic. What blocks a +*commercial* 1.0 is a thin layer of (a) real reliability bugs, (b) naive persistence, (c) built-but-unwired +features, (d) first-run/perceived-quality gaps, and (e) the absence of enforcement/observability tooling. +None require rearchitecting. ---- +## Root-cause themes (the 385 findings collapse to 8) -## Findings +1. **Built-but-unwired features** — command preview (M5), incognito (M6), per-download options/extraArgs + (UX1) are plumbed end-to-end with no UI; the ROADMAP even marks them ✅ (M25). *Decide wire-or-cut.* +2. **"Two ways to do X" with no enforcement** — persistence (M1), validation (CC9), async (CC5), errors + (CC6), UI primitives (UI14/UI18/UI19), formatters (M9) — and no ESLint/Prettier to hold any line (CC2). +3. **Renderer-optimistic state never reconciles + a store cycle** — M34/L142 (settings show unsaved + values), C2 (downloads↔sources circular import). +4. **Naive JSON persistence** — non-atomic writes (R1), corrupt→silent total data loss (R2), O(n²) + full-file rewrites per completion (R3/PERF7), no cache. +5. **Download-engine reliability gaps** — no stall timeout (B1), reverse-order batches (M32), orphan + `.part` on cancel (R4), retry-during-teardown race (L140). +6. **First-run / perceived quality** — light-default theme (SR1), nightly-default yt-dlp (SR2), placeholder + icon (W14), stuck "Resolving…" (SR6), progress bar visibly restarting (SR7), unsigned build (SIGNING.md). +7. **Accessibility & Windows-native feel** — focus rings (UI28/29), no list keyboard nav (W7), title-bar + theme (W3), text-field context menu (W4), no `aria-live` (W17), input labels (M28). +8. **No enforcement / observability** — no lint (CC2), `noImplicitAny:false` (M38), no logging (CC8/M29), + no production source maps (L170). -### Security +## Release gate -#### S1 — Backup import enables arbitrary custom-command templates silently +**MUST fix before 1.0 (blockers)** — correctness, data safety, security, trust: +B1 · M32 · M35 · M34 · R1 · R2 · R3 · H7 · H8 · code-signing · SR1 · SR6 · SR7 · W14. -**File:** [backup.ts:54-55](src/main/backup.ts) -**Severity:** Medium -**Status:** Fixed — 2026-06-23 +**SHOULD fix for 1.0 (high):** wire-or-cut the dead features (M5/M6/UX1) · a11y cluster (UI28/29, W4, W7, +W17, M28) · W3 native theming · lint + `noImplicitAny` (CC2/M38) · dev-jargon copy (M37/SR9) · destructive +confirmations (UX4) · PERF1/PERF2 (per-download redundant work). -**Description:** -`importBackup` restores settings + templates from a user-chosen JSON file. The code validates only that `CommandTemplate` fields have the right shape (id, name, args), then immediately applies them via `setSettings`. If the backup contains `customCommandEnabled: true` + `defaultTemplateId` pointing to a template with `args: "--exec 'cmd'"`, the next download will spawn arbitrary code with no user confirmation that the file carried custom commands. +**DEFER to 1.x:** the ~150 Low items · the simplification refactors (SIMP*, do incrementally) · the UI +token system + shared primitives (UI/SIMP — high value, larger effort) · i18n · sqlite migration. -**Why it matters:** -An attacker-supplied backup file smuggles code execution into an otherwise innocuous "import my settings" gesture. Unlike direct UI creation of templates, there's no moment where the user sees what's being enabled. +## By dimension (severity · reasoning · fix · benefit) -**Fix:** -Surface the count of imported custom commands and require explicit confirmation before applying. Consider importing templates in a disabled state. Alternative: show a preview of template names/args (first 100 chars) before committing. - ---- - -#### S2 — Cookie login window allows popups of arbitrary schemes - -**File:** [cookies.ts:119-125](src/main/cookies.ts) -**Severity:** Low -**Status:** Fixed — 2026-06-23 - -**Description:** -The login window's `setWindowOpenHandler` returns `action: 'allow'` for every popup without protocol filtering. A logged-in webpage could spawn windows with `file://`, custom protocols, or other non-HTTP schemes in the shared session partition. The main window [index.ts:118-126](src/main/index.ts) correctly restricts to `http:`/`https:` only. - -**Why it matters:** -Defense-in-depth: the sandbox limits damage, but an attacker page could open a `file://` popup to exfil cookies to disk, or use a custom-protocol popup as a pivot. - -**Fix:** -Apply the same protocol check to login popups: -```typescript -win.webContents.setWindowOpenHandler((details) => { - try { - const { protocol } = new URL(details.url) - if (protocol !== 'http:' && protocol !== 'https:') return { action: 'deny' } - } catch { return { action: 'deny' } } - return { action: 'allow', ... } -}) -``` - ---- - -#### S3 — Concurrency cap is renderer-only (defense-in-depth gap) - -**File:** [downloads.ts:249-265](src/renderer/src/store/downloads.ts) -**Severity:** Low -**Status:** Fixed — 2026-06-23 - -**Description:** -`maxConcurrent` is enforced solely by the renderer's `pump()` function. `startDownload` in main ([download.ts:214](src/main/download.ts)) only checks for duplicate IDs, not active count. A buggy or compromised renderer could spawn unbounded yt-dlp processes. - -**Why it matters:** -Defense-in-depth: the current design assumes the renderer is trusted. If it ever leaks (XSS in an injected thumbnail URL, future bundled library), the main process should still cap spawns. - -**Fix:** -Add a main-process active-download count check in `startDownload`: -```typescript -if (active.size >= settings.maxConcurrent) { - return { ok: false, error: 'Max concurrent downloads reached. Wait for a slot.' } -} -``` - ---- - -#### S4 — `filenameTemplate` and `outputDir` have no path-traversal checks - -**File:** [settings.ts:150-157](src/main/settings.ts) -**Severity:** Low -**Status:** Fixed — 2026-06-23 - -**Description:** -These fields are validated as `typeof === 'string'` only. A template like `%(title)s\..\..\..\win32.exe.%(ext)s` is joined into `-o` ([download.ts:195](src/main/download.ts)) and permits yt-dlp to write outside the intended folder. Also, `proxy` may carry plaintext credentials (user:pass@...) that get serialized by `exportBackup`. - -**Why it matters:** -On a single-user machine, self-inflicted. But on a shared PC (the app's stated target use case), directory traversal lets one user overwrite another's files. - -**Fix:** -Sanitize `outputDir` (must be absolute, within a reasonable parent) and `filenameTemplate` (reject `..` and absolute paths). For proxy, either mask credentials in backups or document that they're stored plaintext. - ---- - -#### S5 — Persisted JSON (history, templates, errorlog) read without per-field validation - -**Files:** -- [history.ts:17-18](src/main/history.ts) -- [templates.ts:16-17](src/main/templates.ts) -- [errorlog.ts:16-17](src/main/errorlog.ts) - -**Severity:** Low -**Status:** Fixed — 2026-06-23 - -**Description:** -All three do `JSON.parse(...) as T[]` with only an `Array.isArray` gate and no per-field validation. A hand-edited or corrupted file yields entries the UI blindly trusts (e.g., a `HistoryEntry` with `filePath: "C:\\bad"` that passes through to `openPath`). Inconsistent with the rigorous validation elsewhere. - -**Why it matters:** -Low immediate risk (malformed entries mostly degrade gracefully). But it's an untrusted-input boundary that doesn't match the codebase's defensive posture. - -**Fix:** -Add schema validators (`ts-json-validator`, `zod`, or lightweight custom checks) for each type: -```typescript -function isValidHistoryEntry(obj: unknown): obj is HistoryEntry { - return obj && typeof obj === 'object' && 'id' in obj && typeof obj.id === 'string' && ... -} -``` - ---- - -### Correctness - -#### C1 — Bundled `ffprobe.exe` was missing — duration-dependent post-processing failed for every user - -**Files:** [resources/bin/README.md](resources/bin/README.md), [binaries.ts](src/main/binaries.ts), [download.ts:235](src/main/download.ts) -**Severity:** High -**Status:** Fixed — 2026-06-23 - -**Description:** -`resources/bin/` shipped `ffmpeg.exe` but **not** `ffprobe.exe`, and the README only documented copying `ffmpeg.exe`. yt-dlp resolves *both* binaries from `--ffmpeg-location `; without `ffprobe.exe` it cannot read media durations, so any post-processor that needs one fails at runtime with `ERROR: Postprocessing: Unable to determine video duration: ffprobe not found`. That breaks `--sponsorblock-remove`, `--force-keyframes-at-cuts`, and `--split-chapters` for **every** user — end-user machines have no system ffprobe either. It slipped past review because thumbnail/crop/metadata post-processing only uses ffmpeg, and typecheck can't see a missing binary. - -**Found by:** -The real-download smoke test ([real-download.integration.test.ts](test/real-download.integration.test.ts)) — the SponsorBlock-remove case failed with exit 1 (`ffprobe not found`) until ffprobe was bundled. Everything else (crop, audio re-encode, container/codec, subs, chapters, restrict-filenames, archive, extra-args) passed. - -**Fix:** -Copied the matching `ffprobe.exe` (same `n8.1.2` LGPL build, verified by SHA-256 against the already-bundled `ffmpeg.exe`) into `resources/bin/`, and updated the README to list it as a required binary alongside `ffmpeg.exe`. The integration suite now also asserts `ffprobe.exe` is present in `beforeAll`. - -**Hardening (done — 2026-06-23):** -`startDownload` now asserts `ffmpeg.exe` and `ffprobe.exe` presence up front ([download.ts](src/main/download.ts)), alongside the existing `yt-dlp.exe` check — a future missing binary returns a clear AeroFetch error naming the file, instead of a cryptic mid-download yt-dlp postprocessing failure. (`getFfprobePath()` added to [binaries.ts](src/main/binaries.ts).) - ---- +### Reliability +- **[Critical] No download stall timeout (B1).** *Reasoning:* `spawn` has no timeout + no `--socket-timeout`; + a dead connection hangs forever and permanently consumes a concurrency slot. *Fix:* `--socket-timeout` + + an idle watchdog that kills+errors via the existing `killTree`. *Benefit:* downloads self-recover on flaky networks. +- **[Critical] Non-atomic writes + silent corruption→data-loss (R1/R2).** *Reasoning:* `writeFileSync` + isn't atomic; a crash mid-write corrupts the store and the next read returns `[]`, silently wiping history/ + sources. *Fix:* atomic write (temp+rename) + back up a corrupt file before resetting, in one `jsonStore`. + *Benefit:* user data survives crashes; no silent loss. +- **[High] Reverse-order batch downloads (M32)** & **history duplicates (M35).** *Reasoning:* `addMany`+`pump` + promote highest-index first; `redownload` mints a new id defeating dedup. *Fix:* reverse the batch enqueue; + dedup history by URL. *Benefit:* playlists download 1→N; clean history. +- **[High] Optimistic state never reconciles (M34).** *Reasoning:* renderer keeps a value main rejected. + *Fix:* apply the validated `Settings` the IPC returns. *Benefit:* the UI never lies about what's saved. ### Performance - -#### P1 — `getSettings()` writes to disk on every read - -**File:** [settings.ts:91-98](src/main/settings.ts) -**Severity:** Medium -**Status:** Fixed — 2026-06-23 - -**Description:** -Every call to `getSettings()` unconditionally runs `s.set('downloadOptions', sanitizeOptions(...))` and (on first run) `s.set('outputDir', ...)`. `getSettings()` is on hot paths: `buildCommand`, notification checks, system-theme bridge updates, several IPC handlers. Since `electron-store` serializes to disk on each `set`, this means synchronous file I/O per settings read. - -**Why it matters:** -Unnecessary disk churn. Settings reads are frequent (before every download, on IPC calls). Visible on slower machines or filesystems. - -**Fix:** -Sanitize once at store init or wrap `set` in a dirty-flag check: -```typescript -export function getSettings(): Settings { - const s = getStore() - const cur = s.store - const sanitized = sanitizeOptions(cur.downloadOptions) - if (sanitized !== cur.downloadOptions) s.set('downloadOptions', sanitized) - // Only write outputDir once, on first launch - if (!cur.outputDir) s.set('outputDir', app.getPath('downloads')) - return s.store -} -``` - ---- - -#### P2 — Every download spawns a redundant metadata probe - -**File:** [download.ts:249](src/main/download.ts) -**Severity:** Medium -**Status:** Fixed — 2026-06-23 - -**Description:** -`startDownload` always calls `probeMeta` (a second yt-dlp process with `--skip-download`) in parallel with the real download to fetch title/channel/duration. However, the renderer typically already probed and passes this metadata via `addFromUrl`'s optional `meta` parameter ([downloads.ts:60-65](src/renderer/src/store/downloads.ts)). That metadata is never forwarded to `startDownload`, so the main process re-fetches it. - -Result: doubled network traffic and process spawns; the async metadata completion can overwrite good titles the renderer already provided. - -**Why it matters:** -Wasted I/O on every download, especially for large playlists (where the renderer pre-probed each entry). On slow connections, observable slowdown. - -**Fix:** -Add optional `meta` fields to `StartDownloadOptions` and use them if present: -```typescript -export interface StartDownloadOptions { - // ... existing fields ... - meta?: DownloadMeta // optional pre-probed metadata -} - -// In download.ts: -let resolvedTitle = opts.meta?.title ?? (await probeMeta(...)) -if (opts.meta) { - send(wc, { type: 'meta', id: opts.id, meta: opts.meta }) -} else { - probeMeta(ytdlp, opts.url).then(...) -} -``` - ---- - -#### P3 — Lowering `maxConcurrent` mid-flight doesn't pause overflow - -**File:** [downloads.ts:249-265](src/renderer/src/store/downloads.ts) -**Severity:** Low (UX) -**Status:** Fixed — 2026-06-23 (documented in code; intentional behaviour) - -**Description:** -`pump()` only gates *future* promotions of queued items. Reducing `maxConcurrent` while N downloads are active leaves all N running (no immediate pause). Acceptable behavior, but not obvious. - -**Why it matters:** -Users may expect "set max to 1" to pause other downloads immediately. Instead, ongoing ones keep running until they finish. - -**Fix:** -Document the behavior, or add a `cancel-overflow` handler if stricter semantics are desired. For now, a comment suffices. - ---- +- **[High] O(n²) media-items rewrite (R3/PERF7).** *Reasoning:* every completion re-reads+rewrites the + whole (≤20k-item) file synchronously. *Fix:* in-memory cache + batched atomic writes (the same `jsonStore`). + *Benefit:* channel downloads stop hitching the main process. +- **[Medium] Per-download redundant work (PERF1/PERF2).** *Reasoning:* `templates.json` read + settings + decrypt on every spawn even when unused. *Fix:* gate `listTemplates()` on the consent flag; cache decrypted + settings. *Benefit:* lower latency/IO per download. +- **[Low] `summarizeQueue`/`pump` O(n) per tick (PERF3/PERF4).** *Fix:* memoize + count-based pump. *Benefit:* scales to large queues. ### Maintainability +- **[High] Triplicated Settings/`Api` + the preview mock (C1).** *Reasoning:* a 4th touch-point per IPC + method; drift-prone. *Fix:* `DEFAULT_SETTINGS` in `@shared`; one typed `mockApi`. *Benefit:* one place to change. +- **[High] Store circular dependency (C2).** *Fix:* a coordinator/event bus. *Benefit:* removes a latent init crash. +- **[Medium] Duplicated persistence/validation/async/formatters (M1/CC9/CC5/M9).** *Fix:* the `lib/` helpers + in SIMP1–SIMP5. *Benefit:* ~−500 LOC and fewer divergence bugs. +- **[Medium] God files (H1).** *Fix:* SettingsView via ``/`` (SIMP10). *Benefit:* 1104→~600 lines, reviewable. -#### M1 — `cleanError` function duplicated +### Consistency +- **[Medium] Two ways to do X** across persistence, status chips, segmented controls, buttons, errors + (M1/UI18/UI14/UI15/CC6). *Fix:* one shared primitive/standard each (see "Recommended single style"). *Benefit:* the app reads as one product. +- **[Low] Naming/copy drift** (CC1, L95/L150, separators L164). *Fix:* the conventions in CC1. *Benefit:* professional finish. -**Files:** -- [download.ts:81-88](src/main/download.ts) -- [probe.ts:139-146](src/main/probe.ts) +### Readability +- **[Medium] Magic strings/numbers + long signatures + deep nesting (CL1/CL2/CL4, L10).** *Fix:* `constants.ts`, + options-objects, extracted helpers. *Benefit:* faster comprehension, fewer transpose bugs (CL2). -**Severity:** Trivial -**Status:** Fixed — 2026-06-23 (extracted to [log.ts](src/main/log.ts)) +### Polish (perceived quality) +- **[High] First-run defaults (SR1/SR2/SR3).** *Reasoning:* light-on-dark first launch, nightly yt-dlp by + default, auto-download-new on. *Fix:* `theme:'system'`, `ytdlpChannel:'stable'`, `autoDownloadNew:false`. + *Benefit:* the first 10 seconds feel native and safe. +- **[High] Stuck "Resolving…" (SR6) & restarting progress bar (SR7).** *Fix:* clear placeholder on error; + weight the two merge phases. *Benefit:* nothing looks hung/glitchy. +- **[High] Placeholder icon + unsigned build (W14, SIGNING.md).** *Fix:* designed icon; purchase + wire a + cert (already env-ready). *Benefit:* no SmartScreen scare; brand credibility. -**Description:** -Identical 8-line error-log parsing function in two places. If the format changes, both need updating. +### User experience +- **[High] Per-download options are unreachable (UX1).** *Reasoning:* must change global settings to tweak + one download. *Fix:* wire the existing `DownloadOptionsForm` into the bar (plumbing exists). *Benefit:* reclaims a whole feature. +- **[High] Destructive actions have no confirmation/undo (UX4).** *Fix:* confirm Clear/Remove/Delete. *Benefit:* prevents data loss. +- **[Medium] Silent failures on Open/Show (UX6)** and **no global download status off the Downloads tab (UX9).** *Fix:* surface errors; sidebar badge. *Benefit:* the app feels responsive and honest. -**Fix:** -Extract to a shared utility, e.g., `src/main/log.ts` or add to `download.ts` and import in `probe.ts`. +### Accessibility & Windows-native (UX-critical for a Windows product) +- **[High] Fragmented/invisible focus + no list keyboard nav (UI28/29, W7).** *Fix:* one focus-ring + roving + list focus + Delete/Ctrl+A. *Benefit:* keyboard- and Narrator-usable. +- **[Medium] Title bar ignores in-app theme (W3); text fields have no context menu (W4); no `aria-live` (W17); + inputs lack accessible names (M28).** *Fix:* sync `themeSource`; add an editing context menu; live regions; + `aria-label`s. *Benefit:* feels like a native, accessible Windows app. + +### Developer experience +- **[High] No enforcement tooling (CC2) + `noImplicitAny:false` (M38).** *Reasoning:* style/type safety rely + on discipline; implicit `any` is allowed. *Fix:* Prettier + typescript-eslint + CI; re-enable `noImplicitAny`. + *Benefit:* the conventions stay true; regressions caught pre-merge. +- **[Medium] No logging + no prod source maps (CC8/M29/L170).** *Fix:* one leveled file logger at every catch; + hidden source maps. *Benefit:* field crashes become diagnosable instead of invisible. +- **[Low] Stale roadmap/docs (M25/M26, L80–L82); release checksum is manual (H8).** *Fix:* reconcile docs; + generate `.sha256` in `build:win`. *Benefit:* trustworthy docs; updates don't silently fail to install. + +## Suggested PR sequence to 1.0 + +1. **Correctness & data safety:** B1, M32, M35, M34, and the cached/atomic `jsonStore` (R1/R2/R3/PERF7). +2. **Security & trust:** H7 (encrypt cookies), H8 (auto-generate checksums), code-signing. +3. **First-run polish:** SR1/SR2/SR3 defaults, SR6/SR7 status, W14 icon, W3 title-bar theme. +4. **Wire-or-cut + a11y + tooling:** UX1 (per-download panel) or remove M5/M6; focus/keyboard (UI28/29, W7, + W4); Prettier+ESLint+`noImplicitAny` (CC2/M38). +5. **Incremental cleanup (post-1.0):** the SIMP refactors and the Low/UI/UX long tail, behind the new lint gate. --- -#### M2 — `parseExtraArgs` has no escape-sequence handling +## Critical -**File:** [buildArgs.ts:112-120](src/main/buildArgs.ts) -**Severity:** Trivial -**Status:** Fixed — 2026-06-23 (limitation documented in code) +- [ ] **C1 — Single source of truth for the IPC mock + Settings defaults.** `const PREVIEW` + redeclared in 8 files; `main.tsx` reimplements the entire `Api` (~180 lines); the full + `Settings` object is hand-maintained in 3 places (`main/settings.ts` `DEFAULTS`, + `renderer/store/settings.ts` `FALLBACK`, `main.tsx` `MOCK_SETTINGS`). Add `DEFAULT_SETTINGS` + to `shared/ipc.ts`, extract the preview mock into one `mockApi.ts` typed as `Api`, centralize + `isPreview`. +- [ ] **C2 — Break the `downloads ↔ sources` store circular dependency.** `downloads.ts` + imports `useSources`; `sources.ts` imports `useDownloads`. Works only via lazy `.getState()`. + Introduce a coordinator/event bus that owns cross-store reactions. -**Description:** -The shell-like split supports single and double quotes but no escape sequences (`\"` or `\'`). A value containing a literal quote can't be expressed, and an unterminated quote falls through to `\S+` and captures the quote itself. The tests cover the happy path; edge cases aren't reachable in practice (the UI doesn't expose raw quote entry). +## High -**Why it matters:** -Limitation of the current design, documented nowhere. +- [ ] **H1 — Decompose god files.** `SettingsView.tsx` (1104, ~11 cards, ~30 selector subs + + ~20 useState), `DownloadBar.tsx` (858, ~17 state hooks), `store/downloads.ts` (614). +- [ ] **H2 — Consolidate scattered utilities.** `youtubeId` ×2; 4 YouTube-URL-parsing variants; + byte/speed/eta/duration formatting across 4 modules. +- [ ] **H3 — Fix `probe.ts → download.ts` dependency direction** (`fmtBytes` import); resolves + with H2's shared formatter. +- [ ] **H4 — Carry raw numbers across the progress boundary.** `DownloadProgress` ships + formatted `speed`/`eta` strings; `queueStats.ts` re-parses them back to numbers. +- [ ] **H5 — History re-download silently changes quality.** `HistoryView.redownload` passes the + stored `quality` (for format-picker downloads this is a *label* like "720p · mp4 · 184 MB") + back as `quality` with no `formatId`; `buildArgs.videoFormat()` has no matching case and falls + back to `bv*+ba/b` (Best). The queued item shows "720p…" while actually fetching Best. +- [ ] **H6 — TerminalView log grows unbounded.** `setLines((ls) => [...ls, …])` with no cap; a + verbose run (`-F`, `--verbose`) streams thousands of lines into React state. Every other + log/list in the app is capped — cap this too (and/or virtualize). +- [ ] **H7 — `cookies.txt` written in plaintext with no restrictive perms.** [cookies.ts](src/main/cookies.ts) + `writeFileSync(getCookiesFilePath(), …)` stores live auth/session cookies unencrypted under + `userData`. In the **portable build** that's `AeroFetch-data/` next to the exe (USB stick / + shared Downloads folder), so anyone with folder access can read a logged-in session. Settings + secrets are DPAPI-encrypted (settings.ts) but cookies are not. Encrypt at rest or document the + exposure for the portable/shared-PC scenario the app explicitly targets. +- [ ] **H8 — Release checksum is a manual step the updater hard-requires.** [updater.ts](src/main/updater.ts) + sets `REQUIRE_CHECKSUM = true` and refuses any update lacking a `.sha256`, but + `build:win` (`electron-vite build && electron-builder --win`) never generates one. Evidence in + `dist/`: only `0.4.1` has `.sha256` files (hand-made); the current `0.5.0` build does not. If a + release is published without manually adding the checksum, **every client's in-app update + fails** ("This release has no checksum … refusing to install"). Generate the `.sha256` in the + build/release script. -**Fix:** -Add a note near `parseExtraArgs`: "No escape-sequence support; quotes cannot nest. For most yt-dlp use cases (proxy URLs, filename patterns), this is sufficient." If a future template needs quotes within quotes, redesign to a proper shlex or JSON-based format. +## Medium + +- [ ] **M1 — Unify JSON persistence.** `sources.ts` has generic `readJsonArray`/`writeJson`; + `history.ts`/`errorlog.ts`/`templates.ts` reimplement it inline. +- [ ] **M2 — Remove dead code: `getDefaultFolder` / `download:default-folder`** (channel + + preload + handler + mock, zero callers). +- [ ] **M3 — Remove duplicated completion side-effect in `store/downloads.ts`** (history write + + `markDownloaded` in both `applyEvent('done')` and the preview ticker). Subsumed by C2. +- [ ] **M4 — Decide queue persistence explicitly.** Scheduler/queue is renderer-memory only; + `saved`/scheduled items don't survive a quit. Persist or document as a non-goal in code. +- [ ] **M5 — Command-preview feature is fully built but unwired.** `CommandPreviewResult` type + + `command:preview` channel + `previewCommand` (download.ts) + `formatCommandLine`/ + `quoteForDisplay` (buildArgs) + preload method all exist, but no renderer component calls + `window.api.previewCommand` (only the `main.tsx` mock references it). Wire it into the + DownloadBar Options panel, or remove the dead chain. +- [ ] **M6 — Private/incognito mode is plumbed but unreachable.** `incognito` flows through + `AddOptions` → `buildItem` → history-skip → the QueueItem "Private" badge, but nothing in the + UI ever sets `incognito: true`. Add the toggle or remove the dead plumbing. +- [ ] **M7 — `newId()` duplicated 3×** (`store/downloads.ts`, `TerminalView.tsx`, + `TemplateManager.tsx`) with divergent fallback prefixes (`item-`/`t-`/`tpl-`). Extract one helper. +- [ ] **M8 — Two download-status→label maps.** `STATUS_BADGE` (QueueItem) and `STATUS_LABEL` + (LibraryView) independently map the same statuses (completed = "Completed" vs "Downloaded"). + One shared map. +- [ ] **M9 — Three time formatters.** `relTime` (LibraryView), `formatWhen` (HistoryView), + `fmtSchedule` (QueueItem) — consolidate into a date util. +- [ ] **M10 — `.url` shortcut parsing duplicated** — `parseUrlFile` (DownloadBar) vs + `readUrlShortcut` (main/deeplink). Different `URL=` extractors for the same file format. +- [ ] **M11 — Inconsistent clipboard access.** Reads use `navigator.clipboard.readText` (paste + button) *and* `window.api.readClipboard` (suggestion watcher); writes use + `navigator.clipboard.writeText` (copy report). Pick one strategy. +- [ ] **M12 — Shared `errorText` style.** `tokens.colorPaletteRedForeground1` is applied inline + 12× across 5 files instead of one class. +- [ ] **M13 — Inconsistent secret-field masking.** `updateToken` uses `type="password"`; `proxy` + (may carry `user:pass@`) and `youtubePoToken` (a token) are plain-text Inputs. +- [ ] **M14 — Settings search mutates React-owned DOM.** The search toggles each card's + `el.style.display` directly; fragile (breaks if a card ever gets a conditional `style`) and + matches on `textContent` incl. hidden text. Prefer state-driven filtering. +- [ ] **M15 — Nested interactive controls in `role="button"` (a11y).** LibraryView's group header + is a `role="button"` div containing `