fix(security): close CodeQL critical command-line-injection and bad-code-sanitization

[jrusso1020]

What

Close three CodeQL findings in shipping code:

• Critical js/command-line-injection in packages/cli/src/commands/upgrade.ts:76 — the npm registry's version response was string-interpolated into execSync("npm install -g hyperframes@${result.latest}").
• Medium js/bad-code-sanitization (×2) in packages/engine/src/services/frameCapture.ts:496, 604 — JSON.stringify(...) of the skip-id array was template-strung into a page.evaluate("…") JS source string.

Why

A poisoned (or simply weirdly-tagged) npm registry response for hyperframes/latest could put shell metacharacters into the version string and execute arbitrary code on hyperframes upgrade --yes. Even with compareVersions(latest, current) upstream, nothing was enforcing that latest was a plain semver token before it reached the shell.

The two page.evaluate call sites in frameCapture.ts worked in practice (JSON-encoded strings inside a JS string context are mostly benign) but the pattern is the textbook CodeQL anti-pattern for code injection into a remote-evaluation channel. The fix is the standard one: pass the array as a bound argument to a function expression, so the data never appears in source position.

How

upgrade.ts:

• Validate result.latest against a strict semver regex before it touches the install path. If the registry hands back something weird, refuse to install and exit non-zero.
• Switch execSync(installCmd, …) to execFileSync("npm", installArgs, { shell: false }). Belt-and-suspenders: even if the version regex were ever relaxed, no shell parses the argv.

frameCapture.ts:

• Introduce a small pollVideosReady(page, skipIds, timeoutMs) helper that uses page.evaluate((skipIdList) => …, skipIds) — the skip-id list flows through Puppeteer's serialized-arg channel, never through a JS source template literal.
• Replace both call sites with the helper. Removes the JSON.stringify injection and dedupes the small bit of duplicated polling-loop code at the same time.

Test plan

• bun run --cwd packages/cli test — 305/305 passing (includes commands/upgrade adjacent tests via utils/autoUpdate.test).
• bun run --cwd packages/engine test — 591/591 passing (includes frameCapture.test, frameCapture-warmupTicks.test, frameCapture-discardWarmup.test, frameCapture-namePolyfill.test).
• bun run --cwd packages/cli build — clean ESM bundle.
• bun run --cwd packages/engine build — TypeScript build clean.
• bunx oxlint/oxfmt clean on both files.
• CodeQL alerts to be auto-closed on next scan: feat(lint): add gsap_css_transform_conflict and sequential_clips_different_tracks rules #105 (critical), chore(skills): rename compose-video/captions to hyperframes-compose/hyperframes-captions #108, fix(cli): align render output naming and add WebM support to studioServer #109 (medium).

Out of scope

The full CodeQL dashboard has 170 alerts, but ~166 of them are duplicates across generated test fixtures (packages/producer/tests/**/output/compiled.html, tests/**/failures/*.html, skills/.../test-corpus/) or false positives on non-security regex (HTML cleanup in cli/capture/, text normalization in cli/whisper/, build-time HTML compiler regex). Those are worth dismissing in bulk with rationale rather than code changes. This PR only addresses the three real findings in shipping code.

🤖 Generated with Claude Code

[jrusso1020]

• ci(security): add advanced CodeQL setup with path-scoped query filters #860
• fix(security): close CodeQL critical command-line-injection and bad-code-sanitization #856 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miguel-heygen]

Review: Security fixes (3 CodeQL findings)

All three fixes are correct:

1. Command injection in upgrade.ts (critical): Belt-and-suspenders — strict semver regex rejects metacharacters + execFileSync with shell: false prevents re-parsing. Both layers are correct.

2. Code injection in frameCapture.ts (x2): JSON.stringify(skipIds) interpolated into page.evaluate() source strings replaced with parameterized page.evaluate(fn, args) via new pollVideosReady helper. Also deduplicates the two identical polling loops — net code reduction.

No functional regressions — video readiness check logic is identical. LGTM.

[vanceingalls]

Verdict note: Intended verdict is APPROVE (see bottom). Posting as COMMENT because the stamp-harness gated the --approve write — Vance can authorize the upgrade or Magi can stamp.

Independent review — Magi already covered the high-level correctness of all three fixes; this is additive on the trust-boundary and edge-case angles.

Strengths

• upgrade.ts:71-83 — belt-and-suspenders is exactly the right shape here: input validation (SAFE_VERSION) at the trust boundary AND execFileSync + shell: false so the second layer survives any future regex relaxation. The inline comment documents the invariant. This is the textbook fix for js/command-line-injection.
• frameCapture.ts:352-371 — the new pollVideosReady helper mirrors pollPageExpression's polling-with-final-check shape (final return check() after deadline), so timing behavior is preserved across both call sites. Also dedupes ~10 lines of identical polling-loop logic — net code reduction on a security fix is rare and good.
• Trust boundary placement is correct: result.latest ultimately comes from data.version in the npm registry JSON (packages/cli/src/utils/updateCheck.ts:64) and is also persisted into the on-disk config cache (updateCheck.ts:67). Validating at the install site catches both the live fetch path AND the cache-read path in one place.

Findings

nit: upgrade.ts:73 — the SAFE_VERSION regex is technically over-permissive vs. SemVer 2.0.0 (accepts empty pre-release identifiers like 1.0.0-., numeric identifiers with leading zeros like 1.0.0-01). Harmless here — none of those tokens contain shell metacharacters, and execFileSync + shell: false is the load-bearing layer anyway — but if you ever want the validator to also serve as a registry-integrity check, the canonical regex is the one in the semver.org README. Not worth a re-spin.

nit: upgrade.ts:83 — installCmd is now a display-only string built from the already-validated installArgs. The variable name installCmd reads like "the command we're about to run," which is no longer true (the actual exec uses installArgs). Renaming to installCmdDisplay would prevent a future reader from mistaking it for the executed form and re-introducing execSync(installCmd, ...). Optional.

nit: frameCapture.ts:618-622 — pollVideosReady's return value is intentionally discarded at the BeginFrame site (preserving the original while {} else fall through behavior), while the screenshot site at line 510-515 of the post-diff file checks if (!videosReady) throw. That asymmetry pre-existed this PR and is correct to preserve in a security-only fix — flagging for the audit trail, not as a finding. Worth a TODO comment that the BeginFrame path silently times out by design (or a separate PR that makes the two paths consistent).

Notes

• Not in this PR but worth a follow-up issue: the on-disk config.latestVersion (updateCheck.ts:67) is read back on the cache hit path without validation. The fix in this PR catches it at the shell boundary, so the live exploit path is closed. But if any future code reads config.latestVersion and constructs a shell command without going through the same gate, the cached value becomes a stored-injection surface. Cheapest defense: validate at write time in updateCheck.ts (refuse to cache a non-semver latest from the registry response), which would also prevent the cache from accreting garbage.
• CodeQL Analyze (javascript-typescript) came back green on the head commit — the three findings will close on next scan as the PR body claims.

Intended verdict: APPROVE
Reasoning: All three CodeQL findings are closed at the right layer with belt-and-suspenders defenses; behavior preservation across both frameCapture call sites is verified; the trust-boundary placement is the right one. Nits are non-blocking.

Review by Vai