Skip to content

fix(cli,core,lint,producer): terminate ffprobe options at every call site - #2917

Open
vanceingalls wants to merge 1 commit into
ffprobe-5-invocationfrom
ffprobe-6-argv-sweep
Open

fix(cli,core,lint,producer): terminate ffprobe options at every call site#2917
vanceingalls wants to merge 1 commit into
ffprobe-5-invocationfrom
ffprobe-6-argv-sweep

Conversation

@vanceingalls

Copy link
Copy Markdown
Collaborator

Stack 6/6, on top of #2916. Completes #2740.

The gap

#2740 added -- to one of nine independent ffprobe invocations, so the bug class it closed stayed open everywhere else — while CI reported it fixed, because the regression test asserts the argv of that single site.

Reproduced on ffprobe 8.1.1 with an asset named -intro.mp4. It probes fine through extractMediaMetadata, then:

call site failure
producer/services/render/audioPadTrim.ts ×2 Missing argument for option 'intro.mp4'mid-render
cli/commands/init.ts same, during hyperframes init
cli/whisper/transcribe.ts ×2 same, during duration probing
cli/utils/webmAlphaCheck.ts same
core/mediaGradeAnalyzer.ts same
producer/plan-parity-analysis.ts same
lint/hevcPreviewLint.ts catches and returns false — a dash-prefixed HEVC preview silently passes the rule

All nine now terminate their options.

The clone that couldn't take the fix

audioPadTrim.ts:436 runFfprobeJson is a near-verbatim copy of the engine's runFfprobe, and structurally cannot add -- itself: callers bake the input path into args. It now:

That duplication is worth collapsing into the engine helper eventually; this PR does not attempt it, since the two have diverged in error-message prefix and caller contract.

Verification

engine 1280, lint 511, core 1431, cli (init/webmAlphaCheck/whisper) 146, producer audioPadTrim 18 — all pass. Typecheck and lint clean across all five packages.

🤖 Generated with Claude Code

…site

#2740 added `--` to one of nine independent ffprobe invocations, so the
bug class it closed stayed open everywhere else while CI reported it
fixed — the regression test asserts the argv of that single site.

Reproduced on ffprobe 8.1.1: an asset named `-intro.mp4` probes fine
through extractMediaMetadata but fails with "Missing argument for option
'intro.mp4'" in audio pad/trim (mid-render), `hyperframes init`, whisper
duration probing and webmAlphaCheck. hevcPreviewLint catches and returns
false, so a dash-prefixed HEVC preview silently passes the lint rule.

Terminated at all of them:
  producer/services/render/audioPadTrim.ts (x2)
  producer/plan-parity-analysis.ts
  cli/commands/init.ts
  cli/utils/webmAlphaCheck.ts
  cli/whisper/transcribe.ts (x2)
  core/mediaGradeAnalyzer.ts
  lint/hevcPreviewLint.ts

audioPadTrim's runFfprobeJson is a near-verbatim clone of the engine's
runFfprobe and structurally cannot add the terminator itself, because
callers bake the input path into `args`. It now asserts the terminator
is present rather than letting a dash-prefixed path through, takes the
same stdio ["ignore", ...] as the engine helper, and redacts its stderr
— it was throwing raw ffprobe output, which echoes the input path, into
logs and telemetry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stack 6/6: Option terminator at every call site — Review

Good sweep. The audioPadTrim assertion and stderr redaction are solid additions on top of the mechanical -- insertion. One gap.

Blocking: missed call site in audioRegression.ts

packages/producer/src/utils/audioRegression.ts:307probeAudioDuration spawns ffprobe with a bare file argument and no -- terminator:

const proc = spawnSync("ffprobe", [
  "-v", "error",
  "-select_streams", "a:0",
  "-show_entries", "stream=duration",
  "-of", "default=noprint_wrappers=1:nokey=1",
  file,           // <-- no --
], { encoding: "utf-8" });

This is used by regression-harness.ts — production regression-detection, not a test. Same vulnerability class as every other site this PR fixes. A file named -intro.mp4 would produce Missing argument for option 'intro.mp4' here too.

The title says "every call site" — this one makes that claim incomplete.

Verified (the 8 sites the PR does cover)

  • cli/commands/init.ts-- added before filePath in execFileSync
  • cli/utils/webmAlphaCheck.ts — same
  • cli/whisper/transcribe.ts — both sites (duration probe + wav check)
  • core/mediaGradeAnalyzer.ts — same
  • lint/hevcPreviewLint.ts — same. Bonus: this one silently passed dash-prefixed HEVC previews before
  • producer/plan-parity-analysis.ts — same
  • producer/services/render/audioPadTrim.ts — both probe sites + assertion guard in runFfprobeJson + stdio: ["ignore", ...] + stderr redaction via redactTelemetryString

Fix the audioRegression site and the "every" claim lands.

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanical -- addition across the eight sites is straightforward; the higher-value change here is runFfprobeJson's new posture — args.includes("--") assertion, stdio: ["ignore", "pipe", "pipe"], and redactTelemetryString on the error path (stderr echoes the input path into logs/telemetry — that redaction wasn't advertised in the title but is a real independent win). A few things worth surfacing.

Blockers

  • packages/producer/src/utils/audioRegression.ts:307 — missed inside this PR's own scope. probeAudioDuration does spawnSync("ffprobe", ["-v", "error", "-select_streams", "a:0", "-show_entries", "stream=duration", "-of", "default=noprint_wrappers=1:nokey=1", file], ...) with no "--" before file. It's reached from regression-harness.ts:1395 via computeAudioResidualRmsDb(rendered, snapshot, ...). The paths are render outputs and snapshot references rather than raw user input, so the real-world exploit surface is thin — but this is exactly the argv-injection class of bug the PR is closing everywhere else, in producer, and the title ("at every call site") plus body ("nine independent ffprobe invocations") reads as an exhaustive fix. Please patch this one too, or explicitly carve it out.

Concerns

  • Out-of-scope, same class — packages/studio-server/src/helpers/mediaValidation.ts:29. validateUploadedMedia runs runner("ffprobe", [..., filePath]) with no "--", called from validateUploadedMediaBufferroutes/files.ts:2148 against uploaded media filenames. This is the most user-controlled path input in the codebase and squarely the case -- is meant to close. Outside the PR's stated fix(cli,core,lint,producer) scope, but understand you may want to bundle it into this stack or immediately follow up rather than leaving it in a state where "we fixed the argv injection everywhere except the upload endpoint" is technically true.
  • Out-of-scope, same class — packages/studio-server/src/helpers/mediaMetadata.ts:196. probeMediaMetadata also missing --. Reached from proxyTranscoder.ts, mediaCodecMap.ts, routes/media.ts — same class, lower urgency than the upload validator but still there.
  • No regression tests locking -- in the eight patched sites. engine/utils/ffprobe.test.ts:557 / :588 assert the terminator's presence — but only for the engine helper. Nothing in cli/core/lint/producer has an argv-shape assertion. Given the argv layout is identical across all eight ([..., "--", filePath]), a shared helper or per-site assertion is cheap to add; without it, a future refactor of any of the eight can silently reintroduce the bug the same way it slipped past #2740. This bug class has already slipped once — worth locking down.

Nits

  • runFfprobeJson's new comment says "callers bake the input path into args (terminated with --)" — a pointer at the two callers (:355, :375) would help the next reader trace the contract.
  • With the assertion in place, runFfprobeJson reads structurally identical to the engine's runFfprobe modulo the assertion and the redaction. Not a blocker for this PR — the body already notes the two have diverged in error-message prefix and caller contract — but as/when the divergence closes further, collapsing them behind a ---terminator-owning wrapper would remove the sibling-drift risk. Follow-up material.

What I didn't verify

  • Whether the redactTelemetryString(..., 2000) cap is calibrated against real ffprobe stderr sizes on the failure modes that reach this helper. Probably fine — 2000 chars is generous for the "container/codec error" family — just noting the constant wasn't derived from a measured maximum.

Review by Rames D Jusso

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head b46e97bcb9558832fae4227d8190abe5fa66090e against stack parent #2916. One blocker:

P1 — the producer sweep is incomplete and nothing pins the fixed argv contract. packages/producer/src/utils/audioRegression.ts:307-319 still invokes ffprobe with file directly after the options, without "--". This is production source used by the regression harness and sits inside the PR’s declared producer scope, so the PR body’s “all nine now terminate their options” / exhaustive fix claim is not true. The seven changed files also add no regression that inspects argv; that is the same test gap that let #2740 leave the bug class open.

Please add the missing terminator and a focused argv regression that exercises a dash-prefixed path (ideally table-driven across the patched seams where practical). A repo-wide audit also finds the same shape in studio-server/src/helpers/mediaValidation.ts:29-37 and mediaMetadata.ts:196-207; current primary callers appear to pass absolute paths, so I am treating those as follow-up defense-in-depth rather than an additional blocker here.

Exact-head CI is terminal green, the PR is mergeable, and there are no unresolved review threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants