[JasonOA888]

Summary — PR 3/5 for #1451

Converts all 21 execSync calls in recording-tools.ts to execFileSync. This is the largest single file in the audit.

Key vulnerabilities fixed

Call	Risk	Fix
osascript ${tmpScroll} (line 346)	Low-Med — temp file path in shell string	execFileSync("/usr/bin/osascript", [tmpScroll]) — argv array
open "${recPath}" (line 519)	Low — file path from findRecording()	execFileSync("open", [recPath]) — argv array
python3 ... ${action} (line 679)	Low — action from z.enum	execFileSync("python3", [script, action]) — argv array
${bin} -filters 2>&1 (line 27)	Low — bin from hardcoded array	execFileSync(bin, ["-filters"]) — argv array

All other conversions

• 14 AppleScript osascript -e calls: split -e clauses into individual argv entries
• 3 python3 record.py calls: static paths as separate argv
• 1 swift scroll-wheel.swift: static args as argv
• 1 ls -t glob: static literal in /bin/sh -c wrapper (same pattern as PR fix(security): convert task-bridge.ts execSync → execFileSync (PR 2/5, #1451) #1462)

Details

• Import: removed execSync, kept only execFileSync
• No behavioral changes — all 1:1 conversions
• Pre-existing execFileSync calls (lines 226, 285, 744+) untouched
• ffmpeg -filters uses stdio: pipe to capture stderr

After this PR

Remaining files per @bassilkhilo-ag2 triage are all Low/no risk:

• voice-agent.ts (4 calls, hardcoded)
• voice-mode-resolver.ts (1 call, hardcoded)
• browser-tools.ts / inline-tools.ts (already mitigated)

This PR closes the substantive security work for #1451.

Part of #1451 systematic hardening.

[sonichi]

LGTM — CI green (2/2). All conversions are correct:

• osascript calls: split multi--e chains into argv array form correctly. Each -e flag and its AppleScript string are separate array elements — this is the right way.
• findFfmpegWithSubtitles: execFileSync(bin, ['-filters']) with stdio: ['pipe', 'pipe', 'pipe'] is correct (both stdout and stderr captured; previously 2>&1 piped stderr through shell).
• ls glob: static literal string → execFileSync('/bin/sh', ['-c', '...']) is the right pattern when glob expansion is needed and no user data is involved.
• python3 calls: argv array form, no shell interpolation.
• open recPath: execFileSync('open', [recPath]) correctly passes the path as a separate arg.

execSync import fully removed — the unused import cleanup is a nice bonus.

Ready to merge.

[liususan091219]

Review — recording-tools.ts execSync → execFileSync (PR 3/5, #1451)

Verdict. Correct and complete. This is the largest file in the sweep (21 conversions) and every one checks out: argv arrays where possible, /bin/sh -c with a static literal only where the glob+pipe genuinely needs a shell, and output handling preserved at each site. The import drops execSync entirely and no execSync calls remain; the pre-existing execFileSync calls are correctly left untouched.

Things I like

1. recording-tools.ts:27 (ffmpeg -filters) done carefully. The old execSync(\${bin} -filters 2>&1`)both interpolatedbin*and* used shell2>&1to fold stderr into stdout (ffmpeg writes-filtersto stderr). The new code drops the shell but **preserves the capture** viastdio: ['pipe','pipe','pipe']+encoding:'utf-8', then .includes('subtitles')`. Subtle and easy to get wrong — losing the stderr capture here would silently break subtitle-ffmpeg detection. Handled correctly.
2. AppleScript -e splitting. Every multi-clause osascript -e '...' -e '...' is split into one argv entry per -e clause, preserving clause ordering exactly. The ~14 osascript conversions are mechanically faithful — the QuickTime play/pause/close and Chrome-scroll sites match the originals 1:1.
3. open "${recPath}" → execFileSync('open', [recPath]). recPath comes from findRecording() (filesystem-derived), a low-but-real interpolation site, now a clean argv boundary.
4. Glob correctly kept under /bin/sh -c with a static literal. ls -t /tmp/sutando-recording-*.mov | grep -v narrated | grep -v subtitled | head -1 needs the shell for glob + two pipes; the command string is constant → no injection surface. Same pattern as #1462. Output handling (.toString().trim()) preserved.
5. python3 record.py calls — argv arrays, with the z.enum action passed as a discrete arg. Even enum-constrained, treating it as argv rather than string-concat is the defense-in-depth right answer. JSON.parse(raw) downstream unaffected since .toString().trim() is preserved.
6. swift src/scroll-wheel.swift 1 → argv split with static args.

Non-blocking observations

1. encoding/.toString() parity is the thing to watch in a file this size, and it holds. Every site that previously did execSync(...).toString() still does .toString() on the new Buffer-returning call; the one site that switched to encoding:'utf-8' (ffmpeg -filters) then uses .includes() directly, which is correct because encoding makes it return a string. No mixed Buffer/string bug slipped in.
2. Path-pinning is mixed — osascript pinned to /usr/bin/osascript, but python3/swift/open are PATH-resolved. Matches the file's prior behavior, so no regression — noting only if a future "pin all binaries" pass happens.
3. Static-literal /bin/sh -c invariant is the same load-bearing constraint as #1462 — safe today, needs care if anyone later interpolates into it.
4. Best-effort try/catch semantics preserved at every site; all stay synchronous execFileSync, so timing/ordering of the inline tools is unchanged.

Cross-PR linkage

PR 3 of the class with #1452 and #1462, and the largest. It reuses #1462's /bin/sh -c + static-literal glob pattern verbatim and the osascript--e-splitting pattern matches #1452's conversions. Consistent with prior repo execFile work (#530, #527). The PR's claim that this "closes the substantive security work for #1451" (remaining files hardcoded/low-risk) matches what I see — the high-value interpolation sites are the open/prepend-image.sh/meetUrl ones already addressed across the three PRs.

Tests

No test ships with this PR. Given 21 sites, most are static and mechanically verifiable from the diff; the two worth a regression test are the ffmpeg -filters stderr-capture (assert subtitles detection still works) and the findRecording glob. Non-blocking — macOS/ffmpeg-runtime-dependent surface, hard to unit-test cheaply, conversions are 1:1.

Ready to merge

Yes. The largest file in the sweep, converted carefully — the one genuinely tricky site (ffmpeg stderr capture) is handled correctly, output parity holds throughout, and shell-required sites are safe-by-construction.

— Lucy (Mac Studio bot)

[bassilkhilo-ag2]

21 execSync calls converted across 5 sites:

• ffmpeg -filters: static binary path from allowlist ["ffmpeg", "/opt/homebrew/bin/ffmpeg", ...] → execFileSync(bin, ["-filters"], {stdio: ["pipe","pipe","pipe"]}). Note: original used 2>&1 to capture stderr into stdout; new captures stdout only. ffmpeg sends its -filters list to stdout so the includes("subtitles") check still works correctly.
• python3 skills/screen-record/scripts/record.py stop → execFileSync("python3", ["skills/screen-record/scripts/record.py", "stop"]) — correct.
• ls -t /tmp/sutando-recording-*.mov ... → execFileSync("/bin/sh", ["-c", "..."]) with static command — correct, glob expansion needs shell.
• osascript ${tmpScroll} → execFileSync("/usr/bin/osascript", [tmpScroll]) — correct, tmpScroll is a path we just wrote, hardcoded to absolute binary.

LGTM.

[sonichi]

LGTM. execSync removed entirely; all calls through execFileSync. Shell-dependent commands via /bin/sh -c, static binaries via argv array. osascript via /usr/bin/osascript with tmpfile as arg. Stand: Echo Act IV Pro

[sonichi]

LGTM. execSync removed entirely; all calls through execFileSync. Shell-dependent commands via /bin/sh -c, static binaries via argv array. osascript via /usr/bin/osascript with tmpfile as arg. Stand: Echo Act IV Pro

[github-actions]

@cla-assistant check