fix(media): require an executable when resolving ffmpeg - #251
Conversation
resolveFfmpeg() took the first candidate that satisfied existsSync, and existsSync answers true for a directory. On a Linux dev machine electron/native/bin/<tag>/ffmpeg is a folder holding the shared libraries 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. The predicate now requires a regular, executable file. Every failure mode is swallowed on purpose: a candidate that is missing, unreadable or not executable is simply not this one, and throwing would let a single bad path deny a later working one. The candidate list and its order are unchanged.
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
Changesffmpeg resolution validation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/media/audioPeaks.test.ts`:
- Around line 63-117: Extend the resolveFfmpeg tests to verify fallback after
invalid candidates: for directory, non-executable, and supported-platform
unreadable OPENSCREEN_FFMPEG_PATH candidates, stage a later executable candidate
under here and assert resolveFfmpeg(here) returns that later path. Keep the
existing rejection coverage while adding the unreadable-file case and ensuring
each scenario uses the environment-path candidate mechanism.
In `@electron/media/audioPeaks.ts`:
- Around line 121-124: Update isExecutableFile so Windows candidates for
OPENSCREEN_FFMPEG_PATH are accepted only when they follow the defined executable
policy, such as requiring an .exe extension, instead of relying on
fsConstants.X_OK. Preserve the existing regular-file and non-Windows checks, and
add a Windows-specific test covering both accepted executable candidates and
rejected regular files.
- Line 123: Update the ffmpeg candidate validation around accessSync to require
read access before selecting a candidate: use the read permission constant
together with the existing execute check, while preserving the current rejection
and candidate-selection flow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6566deea-0783-4fb6-aade-7a0011641a70
📒 Files selected for processing (2)
electron/media/audioPeaks.test.tselectron/media/audioPeaks.ts
| // A no-op for the caller on Windows, where X_OK is not enforced — but the | ||
| // `isFile` check above is what matters there anyway. | ||
| accessSync(candidate, fsConstants.X_OK); | ||
| return true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching audioPeaks.ts:"
fd -a 'audioPeaks\.ts$' . || true
file=$(fd 'audioPeaks\.ts$' . | head -n 1)
if [ -n "${file:-}" ]; then
echo "Selected: $file"
wc -l "$file"
echo "--- outline ---"
ast-grep outline "$file" || true
echo "--- relevant lines 1-180 ---"
sed -n '1,180p' "$file" | nl -ba
fi
echo "--- references to isExecutableFile, OPENSCREEN_FFMPEG_PATH, audioPeaks ---"
rg -n "isExecutableFile|OPENSCREEN_FFMPEG_PATH|audioPeaks" . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: getopenscreen/openscreen
Length of output: 1292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- relevant lines 1-180 ---"
awk '{print NR"\t"$0}' ./electron/media/audioPeaks.ts | sed -n '1,180p'
echo "--- references to isExecutableFile, OPENSCREEN_FFMPEG_PATH, audioPeaks, ffmpegCandidates, resolveFfmpeg ---"
rg -n "isExecutableFile|OPENSCREEN_FFMPEG_PATH|ffmpegCandidates|resolveFfmpeg|getAudioPeaks" . --glob '!node_modules' --glob '!dist' --glob '!build' || true
echo "--- node availability and fs.X_OK probe ---"
if command -v node >/dev/null 2>&1; then
node - <<'JS'
const fs = require('fs');
const os = require('os');
const path = require('path');
const tmpdir = os.tmpdir();
const file = path.join(tmpdir, 'probe-ffmpeg.txt');
let success = false;
try {
if (fs.existsSync(file)) fs.unlinkSync(file);
fs.writeFileSync(file, 'probe');
try {
fs.accessSync(file, fs.constants.X_OK);
console.log('X_OK_ACCESS=passed');
} catch (err) {
console.log('X_OK_ACCESS=failed:' + err.code);
}
success = true;
} finally {
try { fs.unlinkSync(file); } catch {}
}
console.log('NODE_PLATFORM=' + process.platform);
console.log('PROCESS_VERSION=' + process.version);
process.exit(success ? 0 : 1);
JS
else
echo "node not available"
fi
echo "--- package/runtime hints ---"
sed -n '1,220p' package.json 2>/dev/null || trueRepository: getopenscreen/openscreen
Length of output: 17178
🌐 Web query:
Node.js fs constants X_OK Windows fs.access X_OK behavior F_OK documentation
💡 Result:
In Node.js, the fs.access function and its associated constants (R_OK, W_OK, X_OK, and F_OK) behave differently on Windows compared to POSIX-compliant systems [1]. The fs.constants.X_OK constant, which is intended to check for execute permissions, has no functional effect on Windows [2][3]. When used with fs.access on Windows, fs.constants.X_OK behaves exactly like fs.constants.F_OK, meaning it only checks whether the file exists [2][3]. Furthermore, the fs.access function is generally considered unreliable for checking fine-grained permissions on Windows because Node.js does not natively account for Windows Access Control Lists (ACLs) [4][5]. In most cases on Windows, fs.access checks for file attributes (such as the read-only flag) rather than actual user permissions, which can lead to results that do not accurately reflect the access rights of the calling process [4][5]. Because of these limitations, it is recommended to avoid using fs.access as a pre-check for file operations like reading, writing, or executing [6][5]. This pattern introduces a "Time-of-Check to Time-of-Use" (TOCTOU) race condition [6][5]. Instead, the standard practice is to attempt the desired operation (e.g., fs.readFile or fs.writeFile) directly and handle any errors that occur if the operation fails [6][5].
Citations:
- 1: About checking directory permissions nodejs/node#44868
- 2: nodejs/node@1e7a7be1ad
- 3: nodejs/node@d976d66cfc
- 4: fs file access constants are nearly useless on Windows nodejs/node#36922
- 5:
fs.accessis not reliable inwindows7nodejs/node#7700 - 6: https://nodejs.org/api/fs.html
Define a Windows executable policy for OPENSCREEN_FFMPEG_PATH.
On Windows, fs.constants.X_OK does not enforce executability, so any regular file at that path passes isExecutableFile because ffmpegCandidates puts it first. Add a Windows-specific check, such as supporting only .exe candidates, and add a matching Windows test.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/media/audioPeaks.ts` around lines 121 - 124, Update isExecutableFile
so Windows candidates for OPENSCREEN_FFMPEG_PATH are accepted only when they
follow the defined executable policy, such as requiring an .exe extension,
instead of relying on fsConstants.X_OK. Preserve the existing regular-file and
non-Windows checks, and add a Windows-specific test covering both accepted
executable candidates and rejected regular files.
| } | ||
| // A no-op for the caller on Windows, where X_OK is not enforced — but the | ||
| // `isFile` check above is what matters there anyway. | ||
| accessSync(candidate, fsConstants.X_OK); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'audioPeaks\.ts$' . || true
echo "== relevant file outline/lines =="
file=$(fd 'audioPeaks\.ts$' . | head -n 1)
if [ -n "$file" ]; then
wc -l "$file"
sed -n '1,180p' "$file" | cat -n
fi
echo "== search access sync usage =="
rg -n "accessSync|candidate|X_OK|R_OK|READABLE|audios\|mediaFiles" -S . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: getopenscreen/openscreen
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect related path checks =="
for f in \
electron/stt/whisperServer.ts \
electron/ipc/handlers.ts \
electron/native-bridge/cursor/recording/pipeWireCursorRecordingSession.ts \
electron/native-bridge/cursor/recording/macNativeCursorRecordingSession.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n -C 4 "accessSync|fsConstants" "$f" || true
fi
done
echo "== behavioral probe for access flags on current sandbox tmp =="
python3 - <<'PY'
import os, stat, tempfile
d = tempfile.mkdtemp(prefix="audiosync_")
p = os.path.join(d, "x")
with open(p, "wb") as f:
f.write(b"ok")
for mode in [(stat.S_IRUSR | stat.S_IXUSR), (stat.S_IRUSR,), (stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)]:
os.chmod(p, mode)
print(f"{oct(mode):14} -> R_OK={bool(os.access(p, os.R_OK))} X_OK={bool(os.access(p, os.X_OK))}")
print("tmpdir", d)
PY
rm -rf /tmp /*audiosync_*Repository: getopenscreen/openscreen
Length of output: 4595
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== behavioral probe for access flags =="
python3 - <<'PY'
import os, stat, tempfile
d = tempfile.mkdtemp(prefix="audiosync_")
p = os.path.join(d, "x")
with open(p, "wb") as f:
f.write(b"ok")
for mode in [(stat.S_IRUSR | stat.S_IXUSR), (stat.S_IRUSR,), (stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)]:
os.chmod(p, mode)
print(f"{oct(mode):14} -> R_OK={bool(os.access(p, os.R_OK))} X_OK={bool(os.access(p, os.X_OK))}")
try:
os.chmod(d, 0o500)
q = os.path.join(d, "y")
with open(q, "wb") as f:
f.write(b"ok")
except Exception as e:
print(type(e).__name__, str(e))
else:
os.chmod(q, 0o000)
print("directory_with_nothing_ ->", f"R_OK={bool(os.access(q, os.R_OK))} X_OK={bool(os.access(q, os.X_OK))}")
os.unlink(q)
PY
echo "== permissions probe =="
node - <<'JS'
const fs = require("node:fs");
const os = require("node:os");
const path = require("node:path");
const d = fs.mkdtempSync(path.join(os.tmpdir(), "audiosync-"));
const p = path.join(d, "x");
fs.writeFileSync(p, "ok");
for (const mode of ["0555", "0444", "0755"]) {
fs.chmodSync(p, mode);
console.log(mode, {
read: fs.existsSync(p) && fs.accessSync(p, fs.constants.R_OK) === undefined,
exec: fs.existsSync(p) && fs.accessSync(p, fs.constants.X_OK) === undefined,
});
}
fs.rmSync(d, { recursive: true, force: true });
JSRepository: getopenscreen/openscreen
Length of output: 364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Node permissions probe =="
node - <<'JS'
const fs = require("node:fs");
const os = require("node:os");
const path = require("node:path");
const d = fs.mkdtempSync(path.join(os.tmpdir(), "audiosync-"));
const p = path.join(d, "x");
fs.writeFileSync(p, "ok");
for (const mode of ["0555", "0444", "0755"]) {
try {
fs.chmodSync(p, mode);
const readOk = fs.accessSync(p, fs.constants.R_OK) === undefined;
const execOk = fs.accessSync(p, fs.constants.X_OK) === undefined;
console.log(mode, { read: readOk, exec: execOk });
} catch (error) {
console.log(mode, `error=${error.code}: ${error.message}`);
}
}
try {
fs.chmodSync(d, "0555");
const q = path.join(d, "y");
fs.writeFileSync(q, "ok");
fs.chmodSync(q, "0000");
console.log("dir_with_irregular_file", {
read: fs.accessSync(q, fs.constants.R_OK) === undefined,
exec: fs.accessSync(q, fs.constants.X_OK) === undefined,
});
} catch (error) {
console.log("dir_with_irregular_file", `error=${error.code}: ${error.message}`);
}
fs.rmSync(d, { recursive: true, force: true });
JSRepository: getopenscreen/openscreen
Length of output: 1231
Require read access before selecting the ffmpeg candidate.
accessSync(candidate, fsConstants.X_OK) accepts 0555 files and rejects 0444 files, so X_OK alone does not implement unreadable-candidate rejection. Check R_OK here before returning this candidate.
Proposed fix
- accessSync(candidate, fsConstants.X_OK);
+ accessSync(candidate, fsConstants.R_OK | fsConstants.X_OK);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| accessSync(candidate, fsConstants.X_OK); | |
| accessSync(candidate, fsConstants.R_OK | fsConstants.X_OK); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/media/audioPeaks.ts` at line 123, Update the ffmpeg candidate
validation around accessSync to require read access before selecting a
candidate: use the read permission constant together with the existing execute
check, while preserving the current rejection and candidate-selection flow.
Rejecting a candidate and carrying on to the next are different properties, and only the second is what swallowing every failure buys. With one candidate staged, a null result was equally consistent with "skipped it" and "gave up on the list". The env override 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. An execute-only binary is asserted to be ACCEPTED: X_OK is deliberately not paired with R_OK, because executing a binary needs the execute bit and an install shipped --x must not be refused. Also corrects the doc comment, which claimed unreadable candidates were rejected. X_OK does not test readability, and should not.
|
Thanks — one of the three was a real gap. Addressed in Accepted: cover fallback after an invalid candidate. This was right, and it caught something worse than missing coverage: the doc comment claimed "throwing out of resolution would let a single bad path deny a later, working one" — a property the tests never exercised. With one candidate staged, Four tests added, all routed through Declined: require Executing a binary needs the execute bit, not the read bit. The premise also inverts: What the finding did expose is that my comment said "unreadable", which Declined: The premise is correct — The defect this PR fixes — a directory being selected — is already fixed on Windows by the The reasoning is now in the code comment so it does not have to be re-derived. |
1 participant
Summary
resolveFfmpeg()took the first candidate that satisfiedexistsSync, andexistsSyncanswers true for a directory. On a Linux dev machineelectron/native/bin/<tag>/ffmpegis a folder holding the shared libraries (libavcodec.so.62and friends) rather than the binary, so resolution picked the folder, every later candidate was skipped, and the failure surfaced much later asspawn … EACCES— a message that blames permissions rather than saying the wrong candidate was chosen.The predicate now requires a regular, executable file (
statSync(...).isFile()plusaccessSync(..., X_OK)). Every failure mode is swallowed on purpose: a candidate that is missing, unreadable or not executable is simply not this one, and throwing would let a single bad path deny a later working one. The candidate list and its ordering are unchanged — only the predicate that accepts one.existsSyncwas used nowhere else in the module, so the import goes with it.Related issue
Refs #
Type of change
Release impact
Desktop impact
Testing
Three tests added, including the exact shape that slipped through — a directory sitting where the executable is looked for, containing a
libavcodec.so.62. The non-executable-file case is covered too, guarded to non-Windows since that is where the bit is enforced.npm run test— 134 files, 1586 passing, 0 failures.decoding a real filenow skips on a checkout without a staged binary, which is what its own comment always intended; before this it failed there instead.npm run lint,npx tsc --noEmit,npx tsc -p tsconfig.test.json --noEmit— clean.electron/native/bin/linux-x64/ffmpegis the offending directory confirmedresolveFfmpeg()now returnsnullwhere it previously returned the directory. That probe was removed afterwards and is not part of the diff.🤖 Generated with Claude Code
Summary by CodeRabbit