AeroFetch/CODE-AUDIT.md

# AeroFetch 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

---

## Executive Summary

AeroFetch's security posture is notably thoughtful:

- **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))

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.

---

## Findings

### Security

#### S1 — Backup import enables arbitrary custom-command templates silently

**File:** [backup.ts:54-55](src/main/backup.ts)  
**Severity:** Medium  
**Status:** Fixed — 2026-06-23

**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.

**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.

**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 <dir>`; 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).)

---

### 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.

---

### Maintainability

#### M1 — `cleanError` function duplicated

**Files:**  
- [download.ts:81-88](src/main/download.ts)  
- [probe.ts:139-146](src/main/probe.ts)

**Severity:** Trivial  
**Status:** Fixed — 2026-06-23 (extracted to [log.ts](src/main/log.ts))

**Description:**  
Identical 8-line error-log parsing function in two places. If the format changes, both need updating.

**Fix:**  
Extract to a shared utility, e.g., `src/main/log.ts` or add to `download.ts` and import in `probe.ts`.

---

#### M2 — `parseExtraArgs` has no escape-sequence handling

**File:** [buildArgs.ts:112-120](src/main/buildArgs.ts)  
**Severity:** Trivial  
**Status:** Fixed — 2026-06-23 (limitation documented in code)

**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).

**Why it matters:**  
Limitation of the current design, documented nowhere.

**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.

---

#### M3 — Release polish: app icon

**File:** [electron-builder.yml:48](electron-builder.yml)  
**Severity:** Cosmetic  
**Status:** Placeholder done — 2026-06-23; a *designed* asset is still wanted before v1.0

**Description:**  
The `icon:` line was commented out pending a final icon asset, so builds fell back to the default Electron icon. Bundled binaries are correctly handled (graceful "not found" errors everywhere).

**Fix (done — placeholder):**  
Added [build/icon.svg](build/icon.svg) — a white download glyph on the teal brand square, mirroring the in-app brand mark ([Onboarding.tsx](src/renderer/src/components/Onboarding.tsx)) — and rasterised it to a multi-size [build/icon.ico](build/icon.ico) (256/128/64/48/32/16) via ImageMagick. The `win.icon` line is now uncommented. Regenerate with:
```
magick -background none build/icon.svg -define icon:auto-resize=256,128,64,48,32,16 build/icon.ico
```
**Still open:** swap the placeholder for a professionally designed icon before cutting v1.0.

---

## Preserved Strengths (Continue)

The following approaches are correct and should be maintained:

- **`assertHttpUrl` at every entry** (url.ts, probe.ts, download.ts, cookies.ts, deeplink.ts): consistent, belt-and-suspenders defense against argument injection
- **`--` terminator** before the URL in all spawned commands: ensures URL is never read as a flag
- **`reveal.ts` extension allowlist** (media + subtitle sidecars only): prevents arbitrary-file execution via compromised renderer
- **Process-tree kill on cancel** (`taskkill /T`): correctly cleans up spawned ffmpeg child
- **Separate cookie partition** for login: prevents the login page from accessing app session data
- **`spawn`/`execFile` without `shell: true`**: avoids shell-injection vulns like `%COMSPEC%` CVEs
- **Type-validated settings writes** before persistence: prevents malformed values reaching the store
- **Comprehensive unit tests** for `buildArgs` logic, including the gnarly ffmpeg crop-quoting case

---

## Test Coverage Gaps

The following security-critical functions lack tests:

- `sanitizeOptions` (settings.ts): all validation rules
- `importBackup` (backup.ts): malformed JSON, missing fields, custom-command ingestion
- `safeOpenPath`/`safeShowInFolder` (reveal.ts): extension allowlist enforcement
- `assertHttpUrl` (url.ts): protocol validation (had minor cases, now fixed)

**Action:** Add integration tests or snapshot tests for these before shipping.

---

## Remediation Roadmap

| ID  | Category | Effort | Priority | Notes | Status |
|-----|----------|--------|----------|-------|--------|
| S1  | Security | 2h     | High     | UX confirmation on custom-command import | **Fixed 2026-06-23** |
| S2  | Security | 15m    | Medium   | Copy protocol check from main window | **Fixed 2026-06-23** |
| S3  | Security | 30m    | Medium   | Add active-count gate in main | **Fixed 2026-06-23** |
| S4  | Security | 1h     | Medium   | Path-traversal sanitization | **Fixed 2026-06-23** |
| S5  | Security | 1.5h   | Low      | Add per-field validators to JSON readers | **Fixed 2026-06-23** |
| P1  | Perf     | 1h     | Medium   | Remove disk writes from `getSettings` hot paths | **Fixed 2026-06-23** |
| P2  | Perf     | 1h     | High     | Forward renderer's pre-probed metadata to main | **Fixed 2026-06-23** |
| P3  | UX       | 15m    | Low      | Document or enforce concurrency semantics | **Fixed 2026-06-23** (documented) |
| M1  | Maint    | 15m    | Trivial  | Extract `cleanError` to shared module | **Fixed 2026-06-23** |
| M2  | Maint    | 15m    | Trivial  | Document `parseExtraArgs` limitations | **Fixed 2026-06-23** |
| M3  | Polish   | 30m    | Trivial  | Add icon asset before release | **Placeholder done 2026-06-23**; designed asset still wanted pre-v1.0 |
| C1  | Correctness | 30m | High  | Bundle missing `ffprobe.exe` + document it (found by smoke test) | **Fixed 2026-06-23** |

---

## Notes

- **Deferred:** No findings warrant an emergency patch. S1 is the highest-impact (social engineering), but requires UI changes. S3/S4 are defense-in-depth; production use is low-risk on single-user machines.
- **Testing:** All changes should be accompanied by test additions (especially security validators).
- **DashMail parallel:** This mirrors DashMail's approach to staged remediation (high-impact first, polish last).