diff --git a/CODE-AUDIT.md b/CODE-AUDIT.md index e0edbe1..d62d59b 100644 --- a/CODE-AUDIT.md +++ b/CODE-AUDIT.md @@ -16,7 +16,7 @@ AeroFetch's security posture is notably thoughtful: - **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. +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. --- @@ -132,6 +132,28 @@ function isValidHistoryEntry(obj: unknown): obj is HistoryEntry { --- +### 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).) + +--- + ### Performance #### P1 — `getSettings()` writes to disk on every read @@ -305,6 +327,7 @@ The following security-critical functions lack tests: | 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) | +| C1 | Correctness | 30m | High | Bundle missing `ffprobe.exe` + document it (found by smoke test) | **Fixed 2026-06-23** | --- diff --git a/ROADMAP.md b/ROADMAP.md index 3812cf2..33c4f82 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -27,8 +27,12 @@ choices end-to-end. Persisted defaults are editable in **Settings → Format & post-processing** via a reusable `DownloadOptionsForm` component, which the download bar also hosts in a collapsible per-download **Options** panel. The main process emits the flags in `buildArgs` (`src/main/download.ts`). All items verified in the UI preview -(typecheck clean). *Caveat: the option flags themselves were validated by reasoning + -typecheck, not yet by live yt-dlp downloads — worth a real-download smoke test pass.* +(typecheck clean). **Real-download smoke test ✅ (2026-06-23):** crop-to-square cover, +audio re-encode (opus), video container + codec preference (mkv + vp9 merge), subtitle +embed (→ mov_text), chapter embed, and SponsorBlock-remove (output duration shrinks by the +cut segments) are now all verified against live yt-dlp + ffmpeg in +[real-download.integration.test.ts](test/real-download.integration.test.ts). That pass +found and fixed a real shipping bug — the bundled `ffprobe.exe` was missing (see CODE-AUDIT C1). - [x] **Playlist downloads.** Probe now uses `yt-dlp -J --flat-playlist` and returns either a single video (with formats) or a flat playlist. The download bar shows a selection @@ -45,7 +49,8 @@ typecheck, not yet by live yt-dlp downloads — worth a real-download smoke test overriding the requested resolution. - [x] **Embed chapters.** `--embed-chapters` toggle. (`--split-chapters` still TODO.) - [x] **Embed thumbnail crop-to-square** for audio (Seal's "crop artwork") via thumbnail - post-processor args. *Implemented; needs a real-download smoke test of the ffmpeg crop recipe.* + post-processor args. *Smoke-tested ✅: the embedded cover comes out a perfect square + (360×360) — the `--ppa` crop recipe survives yt-dlp's shlex split + ffmpeg's filtergraph.* - [x] **Per-download overrides.** Collapsible Options panel in the download bar (shared `DownloadOptionsForm`) so one download — or one playlist batch — can deviate from the persisted defaults; "Reset to defaults" clears it. Threads `options` through @@ -55,11 +60,13 @@ typecheck, not yet by live yt-dlp downloads — worth a real-download smoke test `buildArgs`'s former `NetworkOptions` param is now `AccessOptions` (still in `src/main/buildArgs.ts`) — it grew past pure networking once cookies and -filename/archive flags joined proxy/rate-limit/aria2c. *Caveat: the sign-in -window's cookie export is implemented per the Electron session-cookie and -Netscape cookie-jar formats and verified by reasoning + typecheck, not yet by -an actual live login → yt-dlp download against a gated video — worth a real -smoke test.* +filename/archive flags joined proxy/rate-limit/aria2c. **Smoke-tested ✅ (2026-06-23):** +`--restrict-filenames` (spaces → underscores) and `--download-archive` (a second run of +the same URL is skipped with no fresh download) are verified end-to-end. *Still untested +live: the sign-in window's cookie export — it needs an interactive login against a gated +video, which can't run unattended, so it stays reasoning + typecheck only. aria2c +(optional, binary not bundled) and proxy (needs a live proxy server) are likewise not +smoke-tested.* - [x] **Cookies.** Settings → Cookies has a source picker: - *From a browser's cookie store* → `--cookies-from-browser ` (chrome/edge/ @@ -101,9 +108,9 @@ download override that default or opt out entirely (`extraArgs` on options/extraArgs resolution, shared by both the real `startDownload` spawn and the new preview path. All items verified in the UI preview + `npm run typecheck` + `npm run test` (11 new unit tests covering `parseExtraArgs`, `formatCommandLine`, and -extraArgs ordering). *Caveat: the extra-args splitting and command construction were -validated by reasoning + tests, not yet by a real yt-dlp run with actual custom flags — -worth a real-download smoke test pass, same as Phase A/B.* +extraArgs ordering). **Smoke-tested ✅ (2026-06-23):** `parseExtraArgs('--write-info-json +--no-mtime')` splits into discrete tokens and the flags reach a live yt-dlp run — the +`.info.json` sidecar is written, confirming custom-command args take effect end-to-end. - [x] **Custom command templates.** Named, editable templates of extra yt-dlp args (`CommandTemplate`), CRUD'd inline via `TemplateManager.tsx`; persisted to diff --git a/resources/bin/README.md b/resources/bin/README.md index a8f1fd3..6d7caee 100644 --- a/resources/bin/README.md +++ b/resources/bin/README.md @@ -1,16 +1,18 @@ # Bundled binaries AeroFetch spawns these from the **main process** and bundles them via -electron-builder `extraResources` (they ship outside the asar archive). They are -**not** committed to git (see `.gitignore`). +electron-builder `extraResources` (they ship outside the asar archive). The +`.exe` binaries are committed to the repo so a fresh clone builds and runs +without a manual download step. -Drop the two required Windows executables here before running `npm run dev` or +Drop the three required Windows executables here before running `npm run dev` or building; `aria2c.exe` is optional: ``` resources/bin/ ├── yt-dlp.exe ├── ffmpeg.exe +├── ffprobe.exe └── aria2c.exe (optional) ``` @@ -20,13 +22,17 @@ resources/bin/ - Grab `yt-dlp.exe`. - Unlicense / public domain. -## ffmpeg.exe +## ffmpeg.exe + ffprobe.exe - Use an **LGPL** build to keep licensing simple, e.g. https://github.com/BtbN/FFmpeg-Builds/releases (pick a `*-lgpl-shared` or `*-lgpl` win64 build) or https://www.gyan.dev/ffmpeg/builds/. -- Copy `ffmpeg.exe` here. Ship the LGPL license text alongside the installer - before release. +- Copy **both** `ffmpeg.exe` and `ffprobe.exe` here — they ship together in the + same archive's `bin/` folder, and their versions must match. yt-dlp needs + `ffprobe.exe` to read media durations; without it, duration-dependent + post-processing fails with "ffprobe not found" (`--sponsorblock-remove`, + `--force-keyframes-at-cuts`, and `--split-chapters` all error out). +- Ship the LGPL license text alongside the installer before release. ## aria2c.exe (optional) diff --git a/resources/bin/ffprobe.exe b/resources/bin/ffprobe.exe new file mode 100644 index 0000000..25eaf66 Binary files /dev/null and b/resources/bin/ffprobe.exe differ diff --git a/src/main/binaries.ts b/src/main/binaries.ts index 79313f2..c71561b 100644 --- a/src/main/binaries.ts +++ b/src/main/binaries.ts @@ -21,6 +21,16 @@ export function getFfmpegPath(): string { return join(getBinDir(), 'ffmpeg.exe') } +/** + * yt-dlp finds ffprobe via --ffmpeg-location (the bin dir), so the app never + * spawns it directly — but it must be present, or duration-aware post-processing + * (SponsorBlock-remove, --force-keyframes-at-cuts, --split-chapters) fails. This + * accessor exists so startDownload can assert its presence up front. + */ +export function getFfprobePath(): string { + return join(getBinDir(), 'ffprobe.exe') +} + /** Optional bundled external downloader; absent unless dropped into resources/bin. */ export function getAria2cPath(): string { return join(getBinDir(), 'aria2c.exe') diff --git a/src/main/download.ts b/src/main/download.ts index ee95fb8..037f6c1 100644 --- a/src/main/download.ts +++ b/src/main/download.ts @@ -2,7 +2,7 @@ import { spawn, execFile, type ChildProcess } from 'child_process' import { existsSync } from 'fs' import { join } from 'path' import { app, BrowserWindow, Notification, type WebContents } from 'electron' -import { getYtdlpPath, getBinDir, getAria2cPath } from './binaries' +import { getYtdlpPath, getBinDir, getAria2cPath, getFfmpegPath, getFfprobePath } from './binaries' import { getSettings, getDownloadArchivePath } from './settings' import { getCookiesFilePath } from './cookies' import { listTemplates } from './templates' @@ -213,6 +213,22 @@ export function startDownload( error: `yt-dlp.exe not found at ${ytdlp}\nDrop it into resources/bin/ (see the README there).` } } + // ffmpeg is used by nearly every download (merge, audio extract, thumbnail/ + // metadata embed) and ffprobe by duration-aware post-processing (SponsorBlock- + // remove, --force-keyframes-at-cuts, --split-chapters). Assert both up front so a + // missing binary is a clear AeroFetch message, not a cryptic mid-download yt-dlp + // postprocessing error (e.g. "Unable to determine video duration: ffprobe not found"). + const missingBins: string[] = [] + if (!existsSync(getFfmpegPath())) missingBins.push('ffmpeg.exe') + if (!existsSync(getFfprobePath())) missingBins.push('ffprobe.exe') + if (missingBins.length > 0) { + return { + ok: false, + error: + `${missingBins.join(' and ')} not found in ${getBinDir()}\n` + + `Add the ffmpeg build's binaries to resources/bin/ (see the README there).` + } + } // Reject anything that isn't an http(s) URL before it reaches yt-dlp's argv. try { assertHttpUrl(opts.url) diff --git a/test/real-download.integration.test.ts b/test/real-download.integration.test.ts index 7fe6ace..2d5aa1b 100644 --- a/test/real-download.integration.test.ts +++ b/test/real-download.integration.test.ts @@ -10,10 +10,10 @@ */ import { describe, it, expect, beforeAll, afterAll } from 'vitest' import { spawnSync } from 'child_process' -import { mkdtempSync, existsSync, rmSync, statSync } from 'fs' +import { mkdtempSync, existsSync, rmSync, statSync, readFileSync } from 'fs' import { tmpdir } from 'os' import { join, resolve } from 'path' -import { buildArgs } from '../src/main/buildArgs' +import { buildArgs, parseExtraArgs } from '../src/main/buildArgs' import { DEFAULT_DOWNLOAD_OPTIONS, type StartDownloadOptions } from '@shared/ipc' const RUN = process.env.AEROFETCH_REAL_DOWNLOAD === '1' @@ -21,6 +21,7 @@ const RUN = process.env.AEROFETCH_REAL_DOWNLOAD === '1' const BIN_DIR = resolve('resources/bin') const YTDLP = join(BIN_DIR, 'yt-dlp.exe') const FFMPEG = join(BIN_DIR, 'ffmpeg.exe') +const FFPROBE = join(BIN_DIR, 'ffprobe.exe') const NO_ACCESS = { proxy: '', rateLimit: '', restrictFilenames: false } interface RunResult { @@ -79,11 +80,47 @@ function probeSourceDuration(url: string): number | undefined { return Number.isFinite(n) ? n : undefined } +interface ProbeStream { + codec_type?: string + codec_name?: string + width?: number + height?: number +} + +/** Structured ffprobe read of the output file: streams + container + chapters. */ +function ffprobeJson(file: string): { + streams?: ProbeStream[] + chapters?: Array<{ tags?: { title?: string } }> + format?: { format_name?: string } +} { + const r = spawnSync( + FFPROBE, + ['-v', 'quiet', '-print_format', 'json', '-show_format', '-show_streams', '-show_chapters', file], + { encoding: 'utf8', windowsHide: true, maxBuffer: 32 * 1024 * 1024 } + ) + try { + return JSON.parse(r.stdout || '{}') + } catch { + return {} + } +} + +/** All streams of a given codec_type ('video' | 'audio' | 'subtitle'). */ +function streamsOfType(file: string, type: string): ProbeStream[] { + return (ffprobeJson(file).streams ?? []).filter((s) => s.codec_type === type) +} + +/** Basename of a path, splitting on either separator (output paths are Windows). */ +function baseName(p: string): string { + return p.split(/[\\/]/).pop() ?? p +} + let outDir: string beforeAll(() => { if (!RUN) return expect(existsSync(YTDLP), `yt-dlp.exe missing at ${YTDLP}`).toBe(true) expect(existsSync(FFMPEG), `ffmpeg.exe missing at ${FFMPEG}`).toBe(true) + expect(existsSync(FFPROBE), `ffprobe.exe missing at ${FFPROBE}`).toBe(true) outDir = mkdtempSync(join(tmpdir(), 'aerofetch-real-')) }) afterAll(() => { @@ -164,4 +201,189 @@ describe.skipIf(!RUN)('real yt-dlp downloads (buildArgs end-to-end)', () => { }, 240_000 ) + + it( + 'audioFormat opus: --audio-format re-encodes to a real .opus stream', + () => { + const opts: StartDownloadOptions = { + id: 'opus', + url: 'https://www.youtube.com/watch?v=jNQXAC9IVRw', + kind: 'audio', + quality: 'Best' + } + const options = { + ...DEFAULT_DOWNLOAD_OPTIONS, + audioFormat: 'opus' as const, + embedThumbnail: false + } + const argv = buildArgs(opts, join(outDir, '%(id)s.%(ext)s'), options, BIN_DIR, NO_ACCESS) + const res = runYtdlp(argv) + if (res.status !== 0) console.error('[opus] stderr:\n' + res.stderr) + expect(res.status, 'yt-dlp should exit 0').toBe(0) + expect(res.filePath, 'after_move path should be printed').toBeTruthy() + expect(res.filePath!.toLowerCase().endsWith('.opus'), 'output ext should be .opus').toBe(true) + + const audio = streamsOfType(res.filePath!, 'audio') + console.log('[opus] audio codecs:', audio.map((s) => s.codec_name)) + expect(audio.length, 'should have one audio stream').toBeGreaterThan(0) + expect(audio[0].codec_name, '--audio-format opus should produce opus').toBe('opus') + }, + 240_000 + ) + + it( + 'video mkv + vp9 preference: merged .mkv carries a vp9 video stream + audio', + () => { + const opts: StartDownloadOptions = { + id: 'mkv-vp9', + url: 'https://www.youtube.com/watch?v=kJQP7kiw5Fk', // serves both vp9 + av01 at 360p + kind: 'video', + quality: '360p' + } + const options = { + ...DEFAULT_DOWNLOAD_OPTIONS, + videoContainer: 'mkv' as const, + preferredVideoCodec: 'vp9' as const, + embedThumbnail: false + } + const argv = buildArgs(opts, join(outDir, '%(id)s.%(ext)s'), options, BIN_DIR, NO_ACCESS) + console.log('[mkv-vp9] argv:', JSON.stringify(argv)) + const res = runYtdlp(argv) + if (res.status !== 0) console.error('[mkv-vp9] stderr:\n' + res.stderr) + expect(res.status, 'yt-dlp should exit 0').toBe(0) + expect(res.filePath, 'after_move path should be printed').toBeTruthy() + expect(res.filePath!.toLowerCase().endsWith('.mkv'), 'output ext should be .mkv').toBe(true) + + const video = streamsOfType(res.filePath!, 'video') + const audio = streamsOfType(res.filePath!, 'audio') + console.log('[mkv-vp9] video:', video.map((s) => s.codec_name), 'audio:', audio.map((s) => s.codec_name)) + expect(video.length, 'should have a video stream').toBeGreaterThan(0) + expect(audio.length, 'merge should pull in a separate audio stream').toBeGreaterThan(0) + expect(video[0].codec_name, 'the vcodec sort should steer to vp9').toBe('vp9') + }, + 240_000 + ) + + it( + 'embed auto-subtitles + chapters (mp4): mov_text subtitle stream and all 3 chapters', + () => { + const opts: StartDownloadOptions = { + id: 'subs-ch', + url: 'https://www.youtube.com/watch?v=jNQXAC9IVRw', // has en auto-captions + 3 chapters + kind: 'video', + quality: '360p' + } + const options = { + ...DEFAULT_DOWNLOAD_OPTIONS, + videoContainer: 'mp4' as const, + embedSubtitles: true, + autoSubtitles: true, + subtitleLanguages: 'en', + embedChapters: true, + embedThumbnail: false + } + const argv = buildArgs(opts, join(outDir, '%(id)s.%(ext)s'), options, BIN_DIR, NO_ACCESS) + console.log('[subs-ch] argv:', JSON.stringify(argv)) + const res = runYtdlp(argv) + if (res.status !== 0) console.error('[subs-ch] stderr:\n' + res.stderr) + expect(res.status, 'yt-dlp should exit 0').toBe(0) + expect(res.filePath, 'after_move path should be printed').toBeTruthy() + + const info = ffprobeJson(res.filePath!) + const subs = (info.streams ?? []).filter((s) => s.codec_type === 'subtitle') + const chapters = info.chapters ?? [] + console.log('[subs-ch] subtitles:', subs.map((s) => s.codec_name), 'chapters:', chapters.length) + expect(subs.length, 'an en auto-caption should be embedded').toBeGreaterThan(0) + expect(subs[0].codec_name, 'mp4 subtitles convert to mov_text').toBe('mov_text') + expect(chapters.length, 'all 3 source chapters should be embedded').toBe(3) + }, + 240_000 + ) + + it( + 'restrictFilenames: output basename is sanitized to a portable ASCII subset', + () => { + const opts: StartDownloadOptions = { + id: 'restrict', + url: 'https://www.youtube.com/watch?v=jNQXAC9IVRw', // title "Me at the zoo" + kind: 'audio', + quality: 'Best' + } + const options = { ...DEFAULT_DOWNLOAD_OPTIONS, audioFormat: 'mp3' as const, embedThumbnail: false } + const access = { ...NO_ACCESS, restrictFilenames: true } + // %(title)s so the spaces in "Me at the zoo" actually exercise the sanitizer. + const argv = buildArgs(opts, join(outDir, '%(title)s.%(ext)s'), options, BIN_DIR, access) + const res = runYtdlp(argv) + if (res.status !== 0) console.error('[restrict] stderr:\n' + res.stderr) + expect(res.status, 'yt-dlp should exit 0').toBe(0) + expect(res.filePath, 'after_move path should be printed').toBeTruthy() + + const base = baseName(res.filePath!) + console.log('[restrict] basename:', base) + expect(base, 'a restricted filename has no spaces').not.toMatch(/\s/) + expect(base, 'restricted to [A-Za-z0-9._-]').toMatch(/^[A-Za-z0-9._-]+$/) + }, + 240_000 + ) + + it( + 'downloadArchive: a second run of the same URL is skipped via the archive', + () => { + const archive = join(outDir, 'archive.txt') + const mk = (id: string): string[] => + buildArgs( + { id, url: 'https://www.youtube.com/watch?v=jNQXAC9IVRw', kind: 'audio', quality: 'Best' }, + join(outDir, 'arch-%(id)s.%(ext)s'), + { ...DEFAULT_DOWNLOAD_OPTIONS, audioFormat: 'mp3' as const, embedThumbnail: false }, + BIN_DIR, + { ...NO_ACCESS, downloadArchivePath: archive } + ) + + const first = runYtdlp(mk('a1')) + if (first.status !== 0) console.error('[archive] first stderr:\n' + first.stderr) + expect(first.status, 'first run should download').toBe(0) + expect(first.filePath, 'first run should produce a file').toBeTruthy() + expect(existsSync(archive), 'archive file should be written').toBe(true) + + const second = runYtdlp(mk('a2')) + expect(second.status, 'second run should still exit 0').toBe(0) + // The id is in the archive, so the second run downloads nothing — and thus + // prints no after_move `path|` line. (yt-dlp's "has already been recorded" + // notice is itself silenced because buildArgs' --print implies --quiet, so + // we assert on the archive contents + the absence of a fresh download.) + expect(second.filePath, 'second run should be skipped (no new download)').toBeFalsy() + const archiveBody = readFileSync(archive, 'utf8') + console.log('[archive] archive.txt:', JSON.stringify(archiveBody.trim())) + expect(archiveBody, 'the archive should record the downloaded video id').toContain('jNQXAC9IVRw') + }, + 240_000 + ) + + it( + 'extraArgs (custom command): parseExtraArgs tokens reach yt-dlp (--write-info-json sidecar)', + () => { + const opts: StartDownloadOptions = { + id: 'extra', + url: 'https://www.youtube.com/watch?v=jNQXAC9IVRw', + kind: 'audio', + quality: 'Best' + } + const options = { ...DEFAULT_DOWNLOAD_OPTIONS, audioFormat: 'mp3' as const, embedThumbnail: false } + const extra = parseExtraArgs('--write-info-json --no-mtime') + expect(extra, 'parseExtraArgs splits into discrete tokens').toEqual([ + '--write-info-json', + '--no-mtime' + ]) + const argv = buildArgs(opts, join(outDir, 'extra-%(id)s.%(ext)s'), options, BIN_DIR, NO_ACCESS, extra) + const res = runYtdlp(argv) + if (res.status !== 0) console.error('[extra] stderr:\n' + res.stderr) + expect(res.status, 'yt-dlp should exit 0').toBe(0) + expect(res.filePath, 'after_move path should be printed').toBeTruthy() + + const infoJson = join(outDir, 'extra-jNQXAC9IVRw.info.json') + console.log('[extra] info.json exists:', existsSync(infoJson)) + expect(existsSync(infoJson), '--write-info-json should write a sidecar').toBe(true) + }, + 240_000 + ) })