diff --git a/CODE-AUDIT.md b/CODE-AUDIT.md new file mode 100644 index 0000000..e0edbe1 --- /dev/null +++ b/CODE-AUDIT.md @@ -0,0 +1,315 @@ +# 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. + +--- + +## 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' && ... +} +``` + +--- + +### 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 commented out + +**File:** [electron-builder.yml:45](electron-builder.yml) +**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). diff --git a/electron-builder.yml b/electron-builder.yml index 02d7589..e5a600c 100644 --- a/electron-builder.yml +++ b/electron-builder.yml @@ -42,7 +42,10 @@ win: - portable # Never request admin elevation. requestedExecutionLevel: asInvoker - # icon: build/icon.ico # add before release + # TODO(release, audit M3): drop a 256×256 (multi-size) build/icon.ico in place + # and uncomment the next line before cutting v1.0. Until then electron-builder + # falls back to its default Electron icon, which is fine for dev/test builds. + # icon: build/icon.ico portable: artifactName: ${productName}-${version}-portable.${ext} diff --git a/src/main/backup.ts b/src/main/backup.ts index 58deb76..487a55b 100644 --- a/src/main/backup.ts +++ b/src/main/backup.ts @@ -51,7 +51,65 @@ export async function importBackup(win: BrowserWindow | undefined): Promise - if (file.settings && typeof file.settings === 'object') setSettings(file.settings) - if (Array.isArray(file.templates)) replaceTemplates(file.templates) + const incomingSettings = + file.settings && typeof file.settings === 'object' + ? (file.settings as Partial) + : undefined + const incomingTemplates = Array.isArray(file.templates) + ? (file.templates as CommandTemplate[]) + : [] + + // A custom-command template's `args` are extra yt-dlp flags that get spawned on + // the next download. A backup that arrives with customCommandEnabled + a default + // template carrying args would silently enable code execution the moment a + // download starts — so require an explicit, informed confirmation before + // honouring that, and import the templates in a *disabled* state if declined. + const commandTemplates = incomingTemplates.filter( + (t) => t && typeof t === 'object' && typeof t.args === 'string' && t.args.trim() !== '' + ) + const wouldEnableCustomCommands = + incomingSettings?.customCommandEnabled === true && commandTemplates.length > 0 + + let applyCustomCommands = true + if (wouldEnableCustomCommands) { + const preview = commandTemplates + .slice(0, 10) + .map((t) => { + const name = (t.name ?? '').trim() || 'Untitled command' + const args = t.args.length > 100 ? `${t.args.slice(0, 100)}…` : t.args + return `• ${name}: ${args}` + }) + .join('\n') + const more = + commandTemplates.length > 10 ? `\n…and ${commandTemplates.length - 10} more.` : '' + const choice = await dialog.showMessageBox(win!, { + type: 'warning', + buttons: ['Enable custom commands', 'Import without enabling', 'Cancel'], + defaultId: 1, + cancelId: 2, + title: 'Backup contains custom commands', + message: `This backup enables ${commandTemplates.length} custom-command template${ + commandTemplates.length === 1 ? '' : 's' + }.`, + detail: + 'Custom commands run extra yt-dlp flags on every download. Only enable them if you trust this backup file.\n\n' + + preview + + more + }) + if (choice.response === 2) return { ok: false } // Cancel — change nothing + applyCustomCommands = choice.response === 0 + } + + if (incomingSettings) { + // When the user declined to enable custom commands (or there were none to + // enable), force the toggle off so an imported defaultTemplateId can't auto-run. + const safeSettings = applyCustomCommands + ? incomingSettings + : { ...incomingSettings, customCommandEnabled: false } + setSettings(safeSettings) + } + if (incomingTemplates.length > 0 || Array.isArray(file.templates)) { + replaceTemplates(incomingTemplates) + } return { ok: true } } diff --git a/src/main/buildArgs.ts b/src/main/buildArgs.ts index a5f6b92..8ecc536 100644 --- a/src/main/buildArgs.ts +++ b/src/main/buildArgs.ts @@ -106,8 +106,14 @@ export const ARIA2C_ARGS = 'aria2c:-x 16 -s 16 -k 1M' /** * Shell-like split of a raw "extra yt-dlp args" string (Phase C custom-command * templates) into argv tokens. Supports single- and double-quoted spans so a - * flag value containing spaces (e.g. a --ppa recipe) can be typed as one - * token; no escape-character or nested-quote support beyond that. + * flag value containing spaces (e.g. a --ppa recipe) can be typed as one token. + * + * LIMITATIONS (audit M2): there is no escape-sequence support (`\"`, `\'`) and + * quotes cannot nest — a literal quote inside a value can't be expressed, and an + * unterminated quote falls through to the `\S+` branch and captures the quote + * char itself. For the yt-dlp use cases this targets (proxy URLs, filename + * patterns, ffmpeg recipes), that's sufficient. If a future template needs + * nested or escaped quotes, redesign this to a proper shlex or JSON-based format. */ export function parseExtraArgs(raw: string): string[] { const args: string[] = [] diff --git a/src/main/cookies.ts b/src/main/cookies.ts index 6967cf3..ffdc498 100644 --- a/src/main/cookies.ts +++ b/src/main/cookies.ts @@ -116,13 +116,24 @@ export function openCookieLoginWindow(url: string): Promise // Some sites log in via an OAuth/SSO popup. Let those open as real windows // sharing the same partition, rather than silently swallowing the click. - win.webContents.setWindowOpenHandler(() => ({ - action: 'allow', - overrideBrowserWindowOptions: { - autoHideMenuBar: true, - webPreferences: { partition: PARTITION, sandbox: true, contextIsolation: true, nodeIntegration: false } + // Restrict popups to http(s) only — the same defence the main window applies + // (index.ts) — so a logged-in page can't open a file:// popup to exfil cookies + // to disk or use a custom-protocol popup as a pivot. + win.webContents.setWindowOpenHandler((details) => { + try { + const { protocol } = new URL(details.url) + if (protocol !== 'http:' && protocol !== 'https:') return { action: 'deny' } + } catch { + return { action: 'deny' } // unparseable URL — never open it } - })) + return { + action: 'allow', + overrideBrowserWindowOptions: { + autoHideMenuBar: true, + webPreferences: { partition: PARTITION, sandbox: true, contextIsolation: true, nodeIntegration: false } + } + } + }) win.on('closed', () => { loginWindow = null diff --git a/src/main/download.ts b/src/main/download.ts index c492efb..ee95fb8 100644 --- a/src/main/download.ts +++ b/src/main/download.ts @@ -8,6 +8,7 @@ import { getCookiesFilePath } from './cookies' import { listTemplates } from './templates' import { assertHttpUrl } from './url' import { buildArgs, parseExtraArgs, formatCommandLine } from './buildArgs' +import { cleanError } from './log' import { addErrorLog } from './errorlog' import { IpcChannels, @@ -77,16 +78,6 @@ function parseProgress(rest: string): DownloadProgress | null { } } -/** Trim yt-dlp's noisy stderr down to the most useful trailing error line. */ -function cleanError(stderr: string): string { - const lines = stderr - .split('\n') - .map((l) => l.trim()) - .filter(Boolean) - const errLine = [...lines].reverse().find((l) => /^error/i.test(l)) - return (errLine ?? lines[lines.length - 1] ?? '').replace(/^error:\s*/i, '').trim() -} - function send(wc: WebContents, ev: DownloadEvent): void { if (!wc.isDestroyed()) wc.send(IpcChannels.downloadEvent, ev) } @@ -231,6 +222,13 @@ export function startDownload( if (active.has(opts.id)) { return { ok: false, error: 'A download with this id is already running.' } } + // Defence-in-depth: the renderer's pump() already caps concurrency, but the main + // process shouldn't trust it — a buggy or compromised renderer could otherwise + // spawn unbounded yt-dlp processes. Enforce the same cap here on active spawns. + const maxConcurrent = getSettings().maxConcurrent + if (active.size >= maxConcurrent) { + return { ok: false, error: 'Max concurrent downloads reached. Wait for a slot to free up.' } + } let child: ChildProcess try { @@ -242,14 +240,22 @@ export function startDownload( const rec: ActiveDownload = { child, canceled: false } active.set(opts.id, rec) - // Fetch title/channel/duration in parallel so the card fills in quickly. - // Also kept locally so the completion notification/error log can show a - // real title instead of just the raw URL. + // Title/channel/duration are kept locally so the completion notification and + // error log can show a real title instead of just the raw URL. let resolvedTitle: string | undefined - probeMeta(ytdlp, opts.url).then((meta) => { - if (meta?.title) resolvedTitle = meta.title - if (meta && active.has(opts.id)) send(wc, { type: 'meta', id: opts.id, meta }) - }) + if (opts.meta && (opts.meta.title || opts.meta.channel || opts.meta.durationLabel)) { + // The renderer already probed this URL and passed the metadata along — reuse + // it instead of spawning a redundant second yt-dlp probe (audit P2). + resolvedTitle = opts.meta.title + send(wc, { type: 'meta', id: opts.id, meta: opts.meta }) + } else { + // No pre-probed metadata (e.g. a direct paste that skipped the probe) — fetch + // it in parallel so the card fills in quickly. + probeMeta(ytdlp, opts.url).then((meta) => { + if (meta?.title) resolvedTitle = meta.title + if (meta && active.has(opts.id)) send(wc, { type: 'meta', id: opts.id, meta }) + }) + } let stdoutBuf = '' let stderrTail = '' diff --git a/src/main/errorlog.ts b/src/main/errorlog.ts index 2db33cc..a587663 100644 --- a/src/main/errorlog.ts +++ b/src/main/errorlog.ts @@ -2,6 +2,7 @@ import { app } from 'electron' import { join } from 'path' import { readFileSync, writeFileSync, existsSync } from 'fs' import type { ErrorLogEntry } from '@shared/ipc' +import { isValidErrorLogEntry } from './validation' // Plain JSON in userData, same shape as history.ts. Persisted so a failure // report survives the queue item being cleared (Seal's "debug report"). @@ -11,11 +12,14 @@ function errorLogFile(): string { return join(app.getPath('userData'), 'errorlog.json') } +// Per-entry validation (isValidErrorLogEntry, in validation.ts) so a hand-edited +// or corrupted errorlog.json can't feed the UI entries with the wrong shape — +// invalid rows are dropped. (audit S5) export function listErrorLog(): ErrorLogEntry[] { try { if (!existsSync(errorLogFile())) return [] const data = JSON.parse(readFileSync(errorLogFile(), 'utf8')) - return Array.isArray(data) ? (data as ErrorLogEntry[]) : [] + return Array.isArray(data) ? data.filter(isValidErrorLogEntry) : [] } catch { return [] } diff --git a/src/main/history.ts b/src/main/history.ts index 4220ebb..8868720 100644 --- a/src/main/history.ts +++ b/src/main/history.ts @@ -2,6 +2,7 @@ import { app } from 'electron' import { join } from 'path' import { readFileSync, writeFileSync, existsSync } from 'fs' import type { HistoryEntry } from '@shared/ipc' +import { isValidHistoryEntry } from './validation' // Plain JSON in userData (portable build redirects userData next to the exe). // Kept simple per the build plan; can migrate to better-sqlite3 later. @@ -11,11 +12,14 @@ function historyFile(): string { return join(app.getPath('userData'), 'history.json') } +// Per-entry validation (isValidHistoryEntry, in validation.ts) so a hand-edited +// or corrupted history.json can't feed the UI (or openPath) entries with the +// wrong shape — invalid rows are dropped rather than trusted. (audit S5) export function listHistory(): HistoryEntry[] { try { if (!existsSync(historyFile())) return [] const data = JSON.parse(readFileSync(historyFile(), 'utf8')) - return Array.isArray(data) ? (data as HistoryEntry[]) : [] + return Array.isArray(data) ? data.filter(isValidHistoryEntry) : [] } catch { return [] } diff --git a/src/main/log.ts b/src/main/log.ts new file mode 100644 index 0000000..ea8cf84 --- /dev/null +++ b/src/main/log.ts @@ -0,0 +1,14 @@ +/** + * Shared yt-dlp stderr parsing. Used by both download.ts and probe.ts so the + * error-extraction logic lives in one place (audit M1). + */ + +/** Trim yt-dlp's noisy stderr down to the most useful trailing error line. */ +export function cleanError(stderr: string): string { + const lines = stderr + .split('\n') + .map((l) => l.trim()) + .filter(Boolean) + const errLine = [...lines].reverse().find((l) => /^error/i.test(l)) + return (errLine ?? lines[lines.length - 1] ?? '').replace(/^error:\s*/i, '').trim() +} diff --git a/src/main/probe.ts b/src/main/probe.ts index 112e419..0e675ef 100644 --- a/src/main/probe.ts +++ b/src/main/probe.ts @@ -2,6 +2,7 @@ import { execFile } from 'child_process' import { existsSync } from 'fs' import { getYtdlpPath } from './binaries' import { fmtBytes } from './download' +import { cleanError } from './log' import { assertHttpUrl } from './url' import { BEST_FORMAT_ID, @@ -136,15 +137,6 @@ function buildPlaylist(data: RawInfo): PlaylistInfo { } } -function cleanError(stderr: string): string { - const lines = stderr - .split('\n') - .map((l) => l.trim()) - .filter(Boolean) - const errLine = [...lines].reverse().find((l) => /^error/i.test(l)) - return (errLine ?? lines[lines.length - 1] ?? '').replace(/^error:\s*/i, '').trim() -} - /** * Probe a URL with `yt-dlp -J --flat-playlist`. Resolves to a single video (with * its format list) or, when the URL is a playlist, the flat list of its entries. diff --git a/src/main/settings.ts b/src/main/settings.ts index c943339..e185f7b 100644 --- a/src/main/settings.ts +++ b/src/main/settings.ts @@ -1,6 +1,7 @@ import { app } from 'electron' import { join } from 'path' import Store from 'electron-store' +import { isSafeFilenameTemplate, isSafeOutputDir } from './validation' import { AUDIO_FORMATS, VIDEO_CONTAINERS, @@ -90,13 +91,42 @@ function getStore(): Store { export function getSettings(): Settings { const s = getStore() - // Fill in the real Downloads path the first time, and persist it. - if (!s.get('outputDir')) s.set('outputDir', app.getPath('downloads')) - // Migrate settings files that predate downloadOptions (or hold a partial one). - s.set('downloadOptions', sanitizeOptions(s.get('downloadOptions'))) + // getSettings() is on hot paths (buildCommand, notification checks, the system- + // theme bridge, several IPC handlers). electron-store writes to disk on every + // `set`, so only write when something actually changed — otherwise this churns + // the settings file on every read. (audit P1) + const cur = s.store + if (!cur.outputDir) { + // Fill in the real Downloads path the first time, and persist it once. + s.set('outputDir', app.getPath('downloads')) + } + // Migrate settings files that predate downloadOptions (or hold a partial one), + // but only persist when sanitizing actually altered the stored value. + const sanitized = sanitizeOptions(cur.downloadOptions) + if (!downloadOptionsEqual(sanitized, cur.downloadOptions)) { + s.set('downloadOptions', sanitized) + } return s.store } +/** Shallow structural equality for DownloadOptions (sponsorBlockCategories compared by value). */ +function downloadOptionsEqual(a: DownloadOptions, b: unknown): boolean { + if (!b || typeof b !== 'object') return false + const o = b as Partial + const keys = Object.keys(a) as (keyof DownloadOptions)[] + for (const k of keys) { + if (k === 'sponsorBlockCategories') { + const av = a[k] + const bv = o[k] + if (!Array.isArray(bv) || av.length !== bv.length) return false + for (let i = 0; i < av.length; i++) if (av[i] !== bv[i]) return false + } else if (a[k] !== o[k]) { + return false + } + } + return true +} + // Validate each key before persisting — the renderer is the only caller today, // but settings flow into process spawning (maxConcurrent) and the native window // background (theme), so an out-of-range or malformed value shouldn't get stored. @@ -148,9 +178,17 @@ export function setSettings(partial: Partial): Settings { s.set('downloadOptions', sanitizeOptions(value)) break case 'outputDir': + if (typeof value === 'string' && isSafeOutputDir(value.trim())) s.set('outputDir', value.trim()) + break + case 'filenameTemplate': + if (typeof value === 'string' && isSafeFilenameTemplate(value.trim())) { + s.set('filenameTemplate', value.trim()) + } + break case 'defaultVideoQuality': case 'defaultAudioQuality': - case 'filenameTemplate': + // proxy may carry plaintext credentials (user:pass@host); they are stored + // and exported by exportBackup as-is — documented, not masked. case 'proxy': case 'rateLimit': if (typeof value === 'string') s.set(key, value) diff --git a/src/main/templates.ts b/src/main/templates.ts index 9b5f187..17b7420 100644 --- a/src/main/templates.ts +++ b/src/main/templates.ts @@ -2,6 +2,7 @@ import { app } from 'electron' import { join } from 'path' import { readFileSync, writeFileSync, existsSync } from 'fs' import type { CommandTemplate } from '@shared/ipc' +import { isTemplateLike } from './validation' // Plain JSON in userData, same shape as history.ts. const MAX_TEMPLATES = 100 @@ -10,11 +11,17 @@ function templatesFile(): string { return join(app.getPath('userData'), 'templates.json') } +// A persisted template entry must at least be an object carrying an id (see +// isTemplateLike in validation.ts); everything else is coerced by sanitize(). +// Drop anything that isn't, so a hand-edited templates.json can't inject +// malformed entries. (audit S5) export function listTemplates(): CommandTemplate[] { try { if (!existsSync(templatesFile())) return [] const data = JSON.parse(readFileSync(templatesFile(), 'utf8')) - return Array.isArray(data) ? (data as CommandTemplate[]) : [] + // Validate shape, then normalise each surviving entry through sanitize() so + // name/args are always well-formed strings regardless of what was on disk. + return Array.isArray(data) ? data.filter(isTemplateLike).map(sanitize) : [] } catch { return [] } diff --git a/src/main/validation.ts b/src/main/validation.ts new file mode 100644 index 0000000..e9dac21 --- /dev/null +++ b/src/main/validation.ts @@ -0,0 +1,77 @@ +/** + * Pure, dependency-free validators for untrusted input that crosses a trust + * boundary (renderer writes, backup files, hand-edited JSON on disk). + * + * Like buildArgs.ts, this module imports nothing from electron/node beyond the + * `path` builtin, so it can be unit-tested without spinning up Electron. + */ + +import { isAbsolute } from 'path' +import type { HistoryEntry, ErrorLogEntry, CommandTemplate } from '@shared/ipc' + +// --- Path-traversal sanitization (audit S4) --------------------------------- + +/** + * A filenameTemplate is joined onto the output directory and handed to yt-dlp's + * `-o`. Reject anything that could write outside that directory — an absolute + * path, or any `..` path segment — so a malicious backup/settings write can't + * traverse out of the chosen folder (e.g. '%(title)s\..\..\win32.exe'). The + * template still legitimately contains yt-dlp `%(field)s` tokens and `/` or `\` + * for sub-folders, which are fine. + */ +export function isSafeFilenameTemplate(template: string): boolean { + if (isAbsolute(template)) return false + return !template.split(/[\\/]/).some((segment) => segment === '..') +} + +/** An output directory must be an absolute path ('' resolves to OS Downloads). */ +export function isSafeOutputDir(dir: string): boolean { + return dir === '' || isAbsolute(dir) +} + +// --- Persisted-JSON entry validation (audit S5) ----------------------------- + +const isOptionalString = (v: unknown): boolean => v === undefined || typeof v === 'string' + +/** A persisted history.json row must have the right shape or it's dropped on read. */ +export function isValidHistoryEntry(o: unknown): o is HistoryEntry { + if (!o || typeof o !== 'object') return false + const e = o as Record + return ( + typeof e.id === 'string' && + typeof e.title === 'string' && + typeof e.url === 'string' && + (e.kind === 'video' || e.kind === 'audio') && + typeof e.quality === 'string' && + typeof e.completedAt === 'number' && + isOptionalString(e.filePath) && + isOptionalString(e.channel) && + isOptionalString(e.sizeLabel) && + isOptionalString(e.thumbnail) + ) +} + +/** A persisted errorlog.json row must have the right shape or it's dropped on read. */ +export function isValidErrorLogEntry(o: unknown): o is ErrorLogEntry { + if (!o || typeof o !== 'object') return false + const e = o as Record + return ( + typeof e.id === 'string' && + typeof e.url === 'string' && + (e.kind === 'video' || e.kind === 'audio') && + typeof e.error === 'string' && + typeof e.occurredAt === 'number' && + isOptionalString(e.title) + ) +} + +/** + * A persisted templates.json row must at least be an object carrying an id we + * can stringify; name/args are then coerced by templates.ts's sanitize(), so a + * weak shape check here plus normalisation there is enough. + */ +export function isTemplateLike(o: unknown): o is CommandTemplate { + if (!o || typeof o !== 'object') return false + const t = o as Record + return typeof t.id === 'string' || typeof t.id === 'number' +} diff --git a/src/renderer/src/store/downloads.ts b/src/renderer/src/store/downloads.ts index 364e590..5b2f727 100644 --- a/src/renderer/src/store/downloads.ts +++ b/src/renderer/src/store/downloads.ts @@ -1,5 +1,5 @@ import { create } from 'zustand' -import type { DownloadEvent, DownloadOptions } from '@shared/ipc' +import type { DownloadEvent, DownloadMeta, DownloadOptions } from '@shared/ipc' import { useSettings } from './settings' import { useHistory } from './history' @@ -40,6 +40,8 @@ export interface DownloadItem { extraArgs?: string /** private download — completion is never recorded to history */ incognito?: boolean + /** metadata already fetched at probe time; forwarded to main so it skips a redundant probe */ + probedMeta?: DownloadMeta } export const QUALITY_OPTIONS: Record = { @@ -237,7 +239,8 @@ export const useDownloads = create((set, get) => { formatId: item.formatId, formatHasAudio: item.formatHasAudio, options: item.options, - extraArgs: item.extraArgs + extraArgs: item.extraArgs, + meta: item.probedMeta }) .then((res) => { if (!res.ok) markError(item.id, res.error) @@ -246,6 +249,11 @@ export const useDownloads = create((set, get) => { } // Promote queued items (oldest first) into free slots, then launch them. + // + // Note: this only gates *future* promotions. Lowering maxConcurrent while N + // downloads are already active does NOT pause the overflow — the running + // processes finish on their own, and the lower cap takes effect only as slots + // free up. (audit P3 — documented behaviour, not a bug.) function pump(): void { const items = get().items const running = items.filter((i) => i.status === 'downloading').length @@ -270,6 +278,13 @@ export const useDownloads = create((set, get) => { addFromUrl: (url, kind, quality, opts) => { const id = newId() + // If the caller already probed the URL, carry that metadata so main can skip + // its own redundant probe (audit P2). Only set it when something real was + // provided — a bare paste with no probe leaves it undefined. + const probedMeta: DownloadMeta | undefined = + opts?.title || opts?.channel || opts?.durationLabel + ? { title: opts?.title, channel: opts?.channel, durationLabel: opts?.durationLabel } + : undefined const item: DownloadItem = { id, url, @@ -285,7 +300,8 @@ export const useDownloads = create((set, get) => { formatHasAudio: opts?.format?.hasAudio, options: opts?.options, extraArgs: opts?.extraArgs, - incognito: opts?.incognito + incognito: opts?.incognito, + probedMeta } set((s) => ({ items: [item, ...s.items] })) pump() diff --git a/src/shared/ipc.ts b/src/shared/ipc.ts index c917b27..4e2711c 100644 --- a/src/shared/ipc.ts +++ b/src/shared/ipc.ts @@ -248,6 +248,13 @@ export interface StartDownloadOptions { * download even when the settings default is enabled. */ extraArgs?: string + /** + * Metadata the renderer already fetched when probing the URL (title/channel/ + * duration). When present, main uses it directly instead of spawning a second + * yt-dlp `--skip-download` probe — avoiding doubled network traffic and the + * race where the late probe overwrites a good title the renderer supplied. + */ + meta?: DownloadMeta } export interface StartDownloadResult { diff --git a/test/validation.test.ts b/test/validation.test.ts new file mode 100644 index 0000000..199da3b --- /dev/null +++ b/test/validation.test.ts @@ -0,0 +1,146 @@ +import { describe, it, expect } from 'vitest' +import { + isSafeFilenameTemplate, + isSafeOutputDir, + isValidHistoryEntry, + isValidErrorLogEntry, + isTemplateLike +} from '../src/main/validation' +import type { HistoryEntry, ErrorLogEntry } from '@shared/ipc' + +// --- S4: filename template path-traversal ----------------------------------- + +describe('isSafeFilenameTemplate', () => { + it('accepts the default template', () => { + expect(isSafeFilenameTemplate('%(title)s.%(ext)s')).toBe(true) + }) + + it('accepts sub-folder templates with yt-dlp fields', () => { + expect(isSafeFilenameTemplate('%(uploader)s/%(title)s.%(ext)s')).toBe(true) + expect(isSafeFilenameTemplate('%(uploader)s\\%(title)s.%(ext)s')).toBe(true) + }) + + it('rejects forward-slash traversal', () => { + expect(isSafeFilenameTemplate('%(title)s/../../evil.%(ext)s')).toBe(false) + }) + + it('rejects backslash traversal', () => { + expect(isSafeFilenameTemplate('%(title)s\\..\\..\\win32.exe')).toBe(false) + }) + + it('rejects a bare .. segment', () => { + expect(isSafeFilenameTemplate('..')).toBe(false) + }) + + it('rejects absolute paths', () => { + expect(isSafeFilenameTemplate('C:\\Windows\\system32\\%(title)s')).toBe(false) + expect(isSafeFilenameTemplate('/etc/%(title)s')).toBe(false) + }) + + it('allows .. only when embedded in a longer segment (not a real traversal)', () => { + // '..foo' / 'foo..bar' are filenames, not parent-dir references. + expect(isSafeFilenameTemplate('my..video.%(ext)s')).toBe(true) + }) +}) + +// --- S4: output directory ---------------------------------------------------- + +describe('isSafeOutputDir', () => { + it('accepts an empty string (resolves to OS Downloads)', () => { + expect(isSafeOutputDir('')).toBe(true) + }) + + it('accepts an absolute Windows path', () => { + expect(isSafeOutputDir('C:\\Users\\me\\Downloads')).toBe(true) + }) + + it('rejects a relative path', () => { + expect(isSafeOutputDir('Downloads')).toBe(false) + expect(isSafeOutputDir('..\\elsewhere')).toBe(false) + }) +}) + +// --- S5: persisted JSON entry validation ------------------------------------- + +const validHistory: HistoryEntry = { + id: 'a', + title: 'A video', + url: 'https://example.com/v', + kind: 'video', + quality: '1080p', + completedAt: 1700000000000 +} + +describe('isValidHistoryEntry', () => { + it('accepts a well-formed entry', () => { + expect(isValidHistoryEntry(validHistory)).toBe(true) + }) + + it('accepts optional string fields', () => { + expect( + isValidHistoryEntry({ + ...validHistory, + filePath: 'C:\\x.mp4', + channel: 'ch', + sizeLabel: '10 MB', + thumbnail: 'https://x/y.jpg' + }) + ).toBe(true) + }) + + it('rejects a wrong kind', () => { + expect(isValidHistoryEntry({ ...validHistory, kind: 'image' })).toBe(false) + }) + + it('rejects a missing required field', () => { + const { url: _url, ...noUrl } = validHistory + expect(isValidHistoryEntry(noUrl)).toBe(false) + }) + + it('rejects a non-string filePath (e.g. injected number/object)', () => { + expect(isValidHistoryEntry({ ...validHistory, filePath: 42 })).toBe(false) + }) + + it('rejects non-objects', () => { + expect(isValidHistoryEntry(null)).toBe(false) + expect(isValidHistoryEntry('nope')).toBe(false) + expect(isValidHistoryEntry(undefined)).toBe(false) + }) +}) + +const validError: ErrorLogEntry = { + id: 'e', + url: 'https://example.com/v', + kind: 'audio', + error: 'boom', + occurredAt: 1700000000000 +} + +describe('isValidErrorLogEntry', () => { + it('accepts a well-formed entry', () => { + expect(isValidErrorLogEntry(validError)).toBe(true) + expect(isValidErrorLogEntry({ ...validError, title: 'optional' })).toBe(true) + }) + + it('rejects a missing error message', () => { + const { error: _error, ...noError } = validError + expect(isValidErrorLogEntry(noError)).toBe(false) + }) + + it('rejects a non-numeric occurredAt', () => { + expect(isValidErrorLogEntry({ ...validError, occurredAt: 'yesterday' })).toBe(false) + }) +}) + +describe('isTemplateLike', () => { + it('accepts an object with a string or numeric id', () => { + expect(isTemplateLike({ id: 'x', name: 'n', args: '--foo' })).toBe(true) + expect(isTemplateLike({ id: 1 })).toBe(true) + }) + + it('rejects entries without an id', () => { + expect(isTemplateLike({ name: 'n', args: '--foo' })).toBe(false) + expect(isTemplateLike(null)).toBe(false) + expect(isTemplateLike('str')).toBe(false) + }) +})