fix(media): require an executable when resolving ffmpeg

electron/media/audioPeaks.test.ts

@@ -1,7 +1,8 @@
 // @vitest-environment node
-import { existsSync } from "node:fs";
+import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
 import path from "node:path";
-import { describe, expect, it } from "vitest";
+import { afterEach, beforeEach, describe, expect, it } from "vitest";
 import { ffmpegCandidates, peakBlockCount, resolveFfmpeg } from "./audioPeaks";
 
 const ROOT = path.resolve(__dirname, "..", "..");
@@ -52,6 +53,143 @@ describe("ffmpeg resolution", () => {
 	it("returns null rather than throwing when nothing is staged", () => {
 		expect(resolveFfmpeg(path.join(ROOT, "does", "not", "exist"))).toBeNull();
 	});
+
+	/**
+	 * The shape that slipped through. A Linux dev checkout can have
+	 * `electron/native/bin/<tag>/ffmpeg` as a DIRECTORY of shared libraries
+	 * rather than the binary; `existsSync` accepted it, resolution stopped
+	 * there, and the failure only surfaced later as `spawn … EACCES`.
+	 */
+	it("skips a candidate that is a directory rather than the binary", () => {
+		const here = mkdtempSync(path.join(tmpdir(), "openscreen-ffmpeg-"));
+		const tag = `${process.platform}-${process.arch}`;
+		const name = process.platform === "win32" ? "ffmpeg-shared.exe" : "ffmpeg";
+		const staged = path.join(here, "electron", "native", "bin", tag, name);
+		try {
+			// A directory sitting exactly where the executable is looked for.
+			mkdirSync(staged, { recursive: true });
+			writeFileSync(path.join(staged, "libavcodec.so.62"), "");
+
+			expect(resolveFfmpeg(here)).toBeNull();
+		} finally {
+			rmSync(here, { recursive: true, force: true });
+		}
+	});
+
+	it("accepts a candidate that is an executable file", () => {
+		const here = mkdtempSync(path.join(tmpdir(), "openscreen-ffmpeg-"));
+		const tag = `${process.platform}-${process.arch}`;
+		const name = process.platform === "win32" ? "ffmpeg-shared.exe" : "ffmpeg";
+		const staged = path.join(here, "electron", "native", "bin", tag, name);
+		try {
+			mkdirSync(path.dirname(staged), { recursive: true });
+			writeFileSync(staged, "", { mode: 0o755 });
+
+			expect(resolveFfmpeg(here)).toBe(staged);
+		} finally {
+			rmSync(here, { recursive: true, force: true });
+		}
+	});
+
+	// Non-executable files are the other half of the predicate, and the check is
+	// only meaningful where the OS enforces the bit.
+	it.runIf(process.platform !== "win32")(
+		"skips a candidate that is a file but not executable",
+		() => {
+			const here = mkdtempSync(path.join(tmpdir(), "openscreen-ffmpeg-"));
+			const staged = path.join(
+				here,
+				"electron",
+				"native",
+				"bin",
+				`${process.platform}-${process.arch}`,
+				"ffmpeg",
+			);
+			try {
+				mkdirSync(path.dirname(staged), { recursive: true });
+				writeFileSync(staged, "", { mode: 0o644 });
+
+				expect(resolveFfmpeg(here)).toBeNull();
+			} finally {
+				rmSync(here, { recursive: true, force: true });
+			}
+		},
+	);
+
+	/**
+	 * REJECTING IS NOT THE SAME AS CONTINUING, and only the second is the
+	 * property the predicate exists for: swallowing every failure is what stops
+	 * one bad path from denying a later working one. The tests above prove the
+	 * first — with a single candidate staged, `null` is equally consistent with
+	 * "skipped it" and "gave up on the whole list".
+	 *
+	 * `OPENSCREEN_FFMPEG_PATH` is the vehicle because `ffmpegCandidates` puts it
+	 * FIRST, so a bad value there is the one case that could shadow every real
+	 * candidate behind it.
+	 */
+	describe("falling through to a later candidate", () => {
+		let here: string;
+		let staged: string;
+
+		beforeEach(() => {
+			here = mkdtempSync(path.join(tmpdir(), "openscreen-ffmpeg-"));
+			staged = path.join(
+				here,
+				"electron",
+				"native",
+				"bin",
+				`${process.platform}-${process.arch}`,
+				process.platform === "win32" ? "ffmpeg-shared.exe" : "ffmpeg",
+			);
+			mkdirSync(path.dirname(staged), { recursive: true });
+			writeFileSync(staged, "", { mode: 0o755 });
+		});
+
+		afterEach(() => {
+			delete process.env.OPENSCREEN_FFMPEG_PATH;
+			rmSync(here, { recursive: true, force: true });
+		});
+
+		it("passes over a leading candidate that does not exist", () => {
+			process.env.OPENSCREEN_FFMPEG_PATH = path.join(here, "nowhere", "ffmpeg");
+
+			expect(resolveFfmpeg(here)).toBe(staged);
+		});
+
+		it("passes over a leading candidate that is a directory", () => {
+			const decoy = path.join(here, "decoy-ffmpeg");
+			mkdirSync(decoy, { recursive: true });
+			writeFileSync(path.join(decoy, "libavcodec.so.62"), "");
+			process.env.OPENSCREEN_FFMPEG_PATH = decoy;
+
+			expect(resolveFfmpeg(here)).toBe(staged);
+		});
+
+		it.runIf(process.platform !== "win32")(
+			"passes over a leading candidate that is not executable",
+			() => {
+				const decoy = path.join(here, "decoy-ffmpeg");
+				writeFileSync(decoy, "", { mode: 0o644 });
+				process.env.OPENSCREEN_FFMPEG_PATH = decoy;
+
+				expect(resolveFfmpeg(here)).toBe(staged);
+			},
+		);
+
+		// An unreadable-but-executable binary is legitimate on Unix, so it must be
+		// ACCEPTED rather than fallen through — `X_OK` is deliberately not paired
+		// with `R_OK`. Skipped as root, for whom access checks always pass.
+		it.runIf(process.platform !== "win32" && process.getuid?.() !== 0)(
+			"still accepts an execute-only binary",
+			() => {
+				const executableOnly = path.join(here, "exec-only-ffmpeg");
+				writeFileSync(executableOnly, "", { mode: 0o111 });
+				process.env.OPENSCREEN_FFMPEG_PATH = executableOnly;
+
+				expect(resolveFfmpeg(here)).toBe(executableOnly);
+			},
+		);
+	});
 });
 
 // Only runs where the binary is actually staged; skipped elsewhere rather than

electron/media/audioPeaks.ts

@@ -1,6 +1,6 @@
 import { spawn } from "node:child_process";
 import { createHash } from "node:crypto";
-import { existsSync } from "node:fs";
+import { accessSync, constants as fsConstants, statSync } from "node:fs";
 import { mkdir, readFile, stat, writeFile } from "node:fs/promises";
 import path from "node:path";
 import { app } from "electron";
@@ -98,10 +98,46 @@ export function ffmpegCandidates(here: string = process.cwd()): string[] {
 
 let cachedFfmpeg: string | null | undefined;
 
-/** First candidate that exists, or null when none does (callers fall back). */
+/**
+ * Whether a candidate is something that can actually be run.
+ *
+ * EXISTENCE IS NOT ENOUGH, and the difference is not academic. `existsSync` was
+ * the test here, and it answers true for a DIRECTORY: on a Linux dev machine
+ * `electron/native/bin/<tag>/ffmpeg` is a folder holding the shared libraries
+ * (`libavcodec.so.62` and friends) rather than the binary, so resolution picked
+ * the folder, every later candidate was skipped, and the failure surfaced much
+ * later as `spawn … EACCES` — a message that blames permissions rather than
+ * saying the wrong candidate was chosen.
+ *
+ * Every failure mode is swallowed on purpose. A candidate that is absent, not a
+ * regular file, or not executable is simply not this one; throwing out of
+ * resolution would let a single bad path deny a later, working one.
+ *
+ * `X_OK` and not `X_OK | R_OK`: executing a binary needs the execute bit, not
+ * the read bit, so an install shipped `--x` is legitimate and must not be
+ * refused. On Windows `X_OK` is not enforced at all — there the `isFile` check
+ * is the whole guard, which is enough for the failure this exists to stop,
+ * since `ffmpegCandidates` only ever proposes `.exe` names of its own.
+ */
+function isExecutableFile(candidate: string): boolean {
+	try {
+		if (!statSync(candidate).isFile()) {
+			return false;
+		}
+		accessSync(candidate, fsConstants.X_OK);
+		return true;
+	} catch {
+		return false;
+	}
+}
+
+/**
+ * First candidate that is an executable file, or null when none is (callers
+ * fall back).
+ */
 export function resolveFfmpeg(here?: string): string | null {
 	if (cachedFfmpeg !== undefined && here === undefined) return cachedFfmpeg;
-	const found = ffmpegCandidates(here).find((p) => existsSync(p)) ?? null;
+	const found = ffmpegCandidates(here).find(isExecutableFile) ?? null;
 	if (here === undefined) cachedFfmpeg = found;
 	return found;
 }