diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 110c23d37f..464fb00c9f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,6 +33,7 @@ jobs: outputs: code: ${{ steps.filter.outputs.code }} cli: ${{ steps.filter.outputs.cli }} + skills: ${{ steps.filter.outputs.skills }} steps: # Force git-based change detection instead of the pull_request REST API. # The API path can fail the whole workflow on transient listFiles @@ -58,6 +59,10 @@ jobs: - "package.json" - "bun.lock" - ".github/workflows/ci.yml" + skills: + - "skills/**" + - "package.json" + - ".github/workflows/ci.yml" build: name: Build @@ -224,6 +229,22 @@ jobs: - run: bun run --cwd packages/core build:hyperframes-runtime - run: bun run --filter '!@hyperframes/producer' test + # Runs the skills' own node:test suites (e.g. media-use), which live outside + # the workspace packages and are not covered by the `test` job above. Gated on + # the `skills` filter so a skills-only PR actually exercises them in CI. + test-skills: + name: Test (skills) + needs: changes + if: needs.changes.outputs.skills == 'true' + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 + with: + node-version: 22 + - run: node --test 'skills/**/*.test.mjs' + cli-npx-shim: name: "CLI: npx shim (${{ matrix.os }})" needs: changes diff --git a/package.json b/package.json index bcb7a3af60..d5f5d222f0 100644 --- a/package.json +++ b/package.json @@ -33,6 +33,7 @@ "format:check": "oxfmt --check .", "knip": "knip", "test:scripts": "node --import tsx --test scripts/validate-release-channel.test.mjs scripts/draft-changelog.test.ts scripts/set-version.test.ts scripts/release-prepare.test.ts scripts/cli-options.test.ts scripts/changelog-weekly.test.ts scripts/claude-plugin-compression.test.ts", + "test:skills": "node --test 'skills/**/*.test.mjs'", "generate:previews": "tsx scripts/generate-template-previews.ts", "generate:catalog-previews": "tsx scripts/generate-catalog-previews.ts", "upload:docs-images": "bash scripts/upload-docs-images.sh", diff --git a/skills/media-use/scripts/eval.mjs b/skills/media-use/scripts/eval.mjs index eb83c96c57..7184c6fc15 100644 --- a/skills/media-use/scripts/eval.mjs +++ b/skills/media-use/scripts/eval.mjs @@ -15,7 +15,7 @@ import { writeFileSync, } from "node:fs"; import { join, basename, resolve, dirname } from "node:path"; -import { execSync } from "node:child_process"; +import { execFileSync } from "node:child_process"; import { tmpdir } from "node:os"; import { fileURLToPath } from "node:url"; @@ -33,11 +33,19 @@ const TEST_BLOCKS = [ "registry/blocks/instagram-follow", ]; -function run(cmd, opts = {}) { +// Run resolve.mjs with args as a literal argv array (no shell), so values +// interpolated from manifest metadata (--intent prompt, --type) can't inject +// shell. Mirrors the execFileSync fix in probe.mjs / heygen-search.mjs. +function run(args, opts = {}) { try { return { ok: true, - output: execSync(cmd, { encoding: "utf8", timeout: 15000, stdio: "pipe", ...opts }).trim(), + output: execFileSync(process.execPath, [RESOLVE_SCRIPT, ...args], { + encoding: "utf8", + timeout: 15000, + stdio: "pipe", + ...opts, + }).trim(), }; } catch (err) { return { ok: false, output: (err.stdout || "") + (err.stderr || ""), code: err.status }; @@ -92,7 +100,7 @@ function evalBlock(blockPath) { } // with media-use: run --adopt - const adoptResult = run(`node "${RESOLVE_SCRIPT}" --adopt --project "${tmp}" --json`); + const adoptResult = run(["--adopt", "--project", tmp, "--json"]); let adopted = { ok: false, adopted: 0, assets: [] }; if (adoptResult.ok) { try { @@ -129,9 +137,7 @@ function evalBlock(blockPath) { if (manifest.length > 0) { const first = manifest[0]; const prompt = first.provenance?.prompt || first.description; - const r = run( - `node "${RESOLVE_SCRIPT}" --type ${first.type} --intent "${prompt}" --project "${tmp}" --json`, - ); + const r = run(["--type", first.type, "--intent", prompt, "--project", tmp, "--json"]); if (r.ok) { try { resolveTest = JSON.parse(r.output); @@ -142,9 +148,15 @@ function evalBlock(blockPath) { } // test resolve miss: try resolving something that doesn't exist - const missResult = run( - `node "${RESOLVE_SCRIPT}" --type bgm --intent "nonexistent query xyz" --project "${tmp}" --json`, - ); + const missResult = run([ + "--type", + "bgm", + "--intent", + "nonexistent query xyz", + "--project", + tmp, + "--json", + ]); let resolveMiss = null; if (!missResult.ok) { try { diff --git a/skills/media-use/scripts/lib/heygen-search.mjs b/skills/media-use/scripts/lib/heygen-search.mjs index 60ef2f7499..d873504eae 100644 --- a/skills/media-use/scripts/lib/heygen-search.mjs +++ b/skills/media-use/scripts/lib/heygen-search.mjs @@ -1,20 +1,26 @@ -import { execSync } from "node:child_process"; +import { execFileSync } from "node:child_process"; export function heygenSearch(subcommand, query, { type, limit = 5, minScore } = {}) { - const q = query.replace(/'/g, "'\\''"); + // execFileSync with an argv array (no shell), so query/type/etc. are passed as + // literal arguments — no quoting tricks, no command injection. subcommand is a + // hardcoded multi-word string (e.g. "audio sounds list"), split into tokens. // Tag the caller via the CLI's allowlisted attribution header (heygen >= v0.1.6). - const parts = [ - `heygen --headers 'X-HeyGen-Client-Source: media-use' ${subcommand} --query '${q}'`, + const args = [ + "--headers", + "X-HeyGen-Client-Source: media-use", + ...subcommand.split(" "), + "--query", + query, ]; - if (type) parts.push(`--type ${type}`); - parts.push(`--limit ${limit}`); + if (type) args.push("--type", type); + args.push("--limit", String(limit)); // Server-side score floor. Honored by `audio sounds list`; the `asset search` // backend rejects it, so only audio providers pass minScore (see image-provider). - if (minScore != null) parts.push(`--min-score ${minScore}`); + if (minScore != null) args.push("--min-score", String(minScore)); let out; try { - out = execSync(parts.join(" "), { + out = execFileSync("heygen", args, { encoding: "utf8", timeout: 15000, stdio: ["pipe", "pipe", "pipe"], diff --git a/skills/media-use/scripts/lib/probe.mjs b/skills/media-use/scripts/lib/probe.mjs index 7528ef0261..27ef28ce7a 100644 --- a/skills/media-use/scripts/lib/probe.mjs +++ b/skills/media-use/scripts/lib/probe.mjs @@ -1,4 +1,4 @@ -import { execSync } from "node:child_process"; +import { execFileSync } from "node:child_process"; import { extname } from "node:path"; const IMAGE_EXT = new Set([".jpg", ".jpeg", ".png", ".gif", ".webp", ".svg", ".ico"]); @@ -8,8 +8,11 @@ export function probe(filePath) { if (ext === ".svg") return { width: null, height: null, duration: null, codec: "svg" }; try { - const raw = execSync( - `ffprobe -v quiet -print_format json -show_format -show_streams "${filePath}"`, + // execFileSync (no shell) so a hostile filename like `"; rm -rf ~; ".png` + // can't break out of the quoting — filePath is passed as a literal argv entry. + const raw = execFileSync( + "ffprobe", + ["-v", "quiet", "-print_format", "json", "-show_format", "-show_streams", filePath], { encoding: "utf8", timeout: 5000 }, ); const info = JSON.parse(raw); diff --git a/skills/media-use/scripts/lib/probe.test.mjs b/skills/media-use/scripts/lib/probe.test.mjs new file mode 100644 index 0000000000..d4d896cc38 --- /dev/null +++ b/skills/media-use/scripts/lib/probe.test.mjs @@ -0,0 +1,31 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, writeFileSync, existsSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { probe } from "./probe.mjs"; + +// Regression for the shell-injection fix: probe() must pass the path as a literal +// argv entry, never through a shell. A filename containing shell metacharacters +// must NOT execute. Under the old execSync(`ffprobe ... "${path}"`) the embedded +// `touch` ran and created the marker; under execFileSync it cannot, regardless of +// whether ffprobe is installed (the injected command never reaches a shell). +test("probe does not execute shell metacharacters in a filename", () => { + const dir = mkdtempSync(join(tmpdir(), "probe-inject-")); + const marker = join(dir, "INJECTED"); + // Slash-free basename (a real on-disk filename) that breaks out of the old + // double-quoted interpolation and would `touch INJECTED` in the cwd. + const evil = join(dir, `clip"; touch INJECTED; echo ".mp4`); + const prevCwd = process.cwd(); + try { + writeFileSync(evil, "not real media"); + process.chdir(dir); // so a leaked `touch INJECTED` would land next to `marker` + const meta = probe(evil); + assert.equal(existsSync(marker), false, "injected `touch` must not have run"); + // Bogus/unreadable media still returns the null-shaped result, never throws. + assert.deepEqual(Object.keys(meta).sort(), ["codec", "duration", "height", "width"]); + } finally { + process.chdir(prevCwd); + rmSync(dir, { recursive: true, force: true }); + } +});