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

The findings below are opportunities to tighten existing defenses and fix performance gaps. None are foundational breaks.

---

Findings

Security

S1 — Backup import enables arbitrary custom-command templates silently

File: backup.ts:54-55
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
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 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:

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
Severity: Low
Status: Fixed — 2026-06-23

Description:
maxConcurrent is enforced solely by the renderer's pump() function. startDownload in main (download.ts:214) 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:

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
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) 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
• templates.ts:16-17
• errorlog.ts:16-17

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:

function isValidHistoryEntry(obj: unknown): obj is HistoryEntry {
  return obj && typeof obj === 'object' && 'id' in obj && typeof obj.id === 'string' && ...
}

---

Performance

P1 — getSettings() writes to disk on every read

File: settings.ts:91-98
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:

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

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
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
• probe.ts:139-146

Severity: Trivial
Status: Fixed — 2026-06-23 (extracted to 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
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 commented out

File: electron-builder.yml:45
Severity: Cosmetic
Status: Deferred — TODO(release) comment added; requires a designed icon asset before v1.0

Description:
The icon: line is commented out pending final icon asset. Bundled binaries are correctly handled (graceful "not found" errors everywhere).

Fix:
Add a build/icon.ico before shipping 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	Deferred (pre-release TODO)

---

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