Skip to content

Commit 6575caf

Browse files
committed
fix(engine): harden ffprobe parsing and command arguments
- parseFrameRate now rejects malformed ratios (e.g. "30/", "30/0") instead of NaN. - Add "--" before file paths so names starting with "-" are not parsed as options. - cICP PNG chunk no longer returns before IHDR supplies width and height. - Add regression tests for option injection, frame rates, and cICP ordering.
1 parent 7a294f1 commit 6575caf

2 files changed

Lines changed: 168 additions & 24 deletions

File tree

packages/engine/src/utils/ffprobe.test.ts

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,3 +404,143 @@ describe("ffprobe missing-binary fallback", () => {
404404
await expect(extractAudioMetadata("/tmp/example.mp3")).rejects.toThrow(/install FFmpeg/i);
405405
});
406406
});
407+
408+
describe("ffprobe option separator", () => {
409+
afterEach(() => {
410+
vi.resetModules();
411+
vi.doUnmock("child_process");
412+
});
413+
414+
it("places -- before the file path so paths starting with - are not parsed as options", async () => {
415+
const { spawn, calls } = createSpawnSpy([
416+
{
417+
kind: "exit",
418+
code: 0,
419+
stdout: JSON.stringify({
420+
streams: [
421+
{
422+
codec_type: "video",
423+
codec_name: "h264",
424+
width: 320,
425+
height: 180,
426+
r_frame_rate: "30/1",
427+
avg_frame_rate: "30/1",
428+
},
429+
],
430+
format: { duration: "1.5" },
431+
}),
432+
},
433+
]);
434+
vi.resetModules();
435+
vi.doMock("child_process", () => ({ spawn }));
436+
437+
const { extractMediaMetadata } = await import("./ffprobe.js");
438+
const filePath = "/tmp/-dangerous-name.mp4";
439+
await extractMediaMetadata(filePath);
440+
441+
const args = calls[0]?.args ?? [];
442+
const filePathIndex = args.indexOf(filePath);
443+
expect(filePathIndex).toBeGreaterThan(0);
444+
expect(args[filePathIndex - 1]).toBe("--");
445+
});
446+
447+
it("uses -- for audio and keyframe probes too", async () => {
448+
const { spawn, calls } = createSpawnSpy([
449+
{
450+
kind: "exit",
451+
code: 0,
452+
stdout: JSON.stringify({
453+
streams: [{ codec_type: "audio", codec_name: "aac", sample_rate: "48000", channels: 2 }],
454+
format: { duration: "1.25" },
455+
}),
456+
},
457+
{
458+
kind: "exit",
459+
code: 0,
460+
stdout: JSON.stringify({
461+
streams: [{ nb_read_packets: "783" }],
462+
format: {},
463+
}),
464+
},
465+
{ kind: "exit", code: 0, stdout: "0.000\n1.000\n" },
466+
]);
467+
vi.resetModules();
468+
vi.doMock("child_process", () => ({ spawn }));
469+
470+
const { extractAudioMetadata, analyzeKeyframeIntervals } = await import("./ffprobe.js");
471+
await extractAudioMetadata("/tmp/-audio.wav");
472+
await analyzeKeyframeIntervals("/tmp/-video.mp4");
473+
474+
const args = calls.flatMap((call) => [...(call.args ?? [])]);
475+
expect(args.filter((arg) => arg === "--")).toHaveLength(3);
476+
});
477+
});
478+
479+
describe("ffprobe frame rate parsing", () => {
480+
afterEach(() => {
481+
vi.resetModules();
482+
vi.doUnmock("child_process");
483+
});
484+
485+
it.each([
486+
{ r: "30/1", avg: "30/1", expected: 30 },
487+
{ r: "30000/1001", avg: "30000/1001", expected: 29.97 },
488+
{ r: "30/", avg: undefined, expected: 0 },
489+
{ r: "30/0", avg: undefined, expected: 0 },
490+
{ r: "0/0", avg: undefined, expected: 0 },
491+
{ r: "abc/def", avg: undefined, expected: 0 },
492+
{ r: "60", avg: undefined, expected: 60 },
493+
])("parses r=$r avg=$avg as fps=$expected", async ({ r, avg, expected }) => {
494+
const { spawn } = createSpawnSpy([
495+
{
496+
kind: "exit",
497+
code: 0,
498+
stdout: JSON.stringify({
499+
streams: [
500+
{
501+
codec_type: "video",
502+
codec_name: "h264",
503+
width: 320,
504+
height: 180,
505+
r_frame_rate: r,
506+
avg_frame_rate: avg,
507+
},
508+
],
509+
format: { duration: "1.5" },
510+
}),
511+
},
512+
]);
513+
vi.resetModules();
514+
vi.doMock("child_process", () => ({ spawn }));
515+
516+
const { extractMediaMetadata } = await import("./ffprobe.js");
517+
const meta = await extractMediaMetadata("/tmp/frame-rate.mp4");
518+
519+
expect(meta.fps).toBe(expected);
520+
});
521+
});
522+
523+
describe("extractPngMetadataFromBuffer cICP ordering", () => {
524+
it("does not emit color space until IHDR provides width and height", () => {
525+
const ihdr = pngChunk("IHDR", [0, 0, 0, 1, 0, 0, 0, 1, 16, 2, 0, 0, 0]);
526+
const cicp = pngChunk("cICP", [9, 16, 0, 1]);
527+
const iend = pngChunk("IEND", []);
528+
529+
// cICP before IHDR is invalid PNG ordering; make sure we don't return
530+
// zero-sized metadata in that case.
531+
const malformed = buildPngWithChunks([cicp, ihdr, iend]);
532+
expect(extractPngMetadataFromBuffer(malformed)).toEqual({
533+
width: 1,
534+
height: 1,
535+
colorSpace: {
536+
colorPrimaries: "bt2020",
537+
colorTransfer: "smpte2084",
538+
colorSpace: "gbr",
539+
},
540+
});
541+
542+
// Without any IHDR, a cICP alone should not produce a result.
543+
const onlyCicp = buildPngWithChunks([cicp, iend]);
544+
expect(extractPngMetadataFromBuffer(onlyCicp)).toBeNull();
545+
});
546+
});

packages/engine/src/utils/ffprobe.ts

Lines changed: 28 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ export function extractPngMetadataFromBuffer(buf: Buffer): StillImageMetadata |
156156
let width = 0;
157157
let height = 0;
158158
let seenIdat = false;
159+
let colorSpaceFromCicp: VideoColorSpace | null = null;
159160
let pos = 8;
160161
while (pos + 12 <= buf.length) {
161162
const chunkLen = buf.readUInt32BE(pos);
@@ -180,35 +181,31 @@ export function extractPngMetadataFromBuffer(buf: Buffer): StillImageMetadata |
180181
const transferCode = chunkData[1] ?? 0;
181182
const matrixCode = chunkData[2] ?? 0;
182183

183-
return {
184-
width,
185-
height,
186-
colorSpace: {
187-
colorPrimaries:
188-
primariesCode === 9
189-
? "bt2020"
190-
: primariesCode === 1
184+
colorSpaceFromCicp = {
185+
colorPrimaries:
186+
primariesCode === 9
187+
? "bt2020"
188+
: primariesCode === 1
189+
? "bt709"
190+
: `unknown-${primariesCode}`,
191+
colorTransfer:
192+
transferCode === 16
193+
? "smpte2084"
194+
: transferCode === 18
195+
? "arib-std-b67"
196+
: transferCode === 1
191197
? "bt709"
192-
: `unknown-${primariesCode}`,
193-
colorTransfer:
194-
transferCode === 16
195-
? "smpte2084"
196-
: transferCode === 18
197-
? "arib-std-b67"
198-
: transferCode === 1
199-
? "bt709"
200-
: `unknown-${transferCode}`,
201-
colorSpace:
202-
matrixCode === 9 ? "bt2020nc" : matrixCode === 0 ? "gbr" : `unknown-${matrixCode}`,
203-
},
198+
: `unknown-${transferCode}`,
199+
colorSpace:
200+
matrixCode === 9 ? "bt2020nc" : matrixCode === 0 ? "gbr" : `unknown-${matrixCode}`,
204201
};
205202
}
206203

207204
if (chunkType === "IEND") break;
208205
pos += 12 + chunkLen;
209206
}
210207

211-
return width > 0 && height > 0 ? { width, height, colorSpace: null } : null;
208+
return width > 0 && height > 0 ? { width, height, colorSpace: colorSpaceFromCicp } : null;
212209
}
213210

214211
function extractStillImageMetadata(filePath: string): StillImageMetadata | null {
@@ -242,9 +239,13 @@ function parseFrameRate(frameRateStr: string | undefined): number {
242239
if (parts.length === 2) {
243240
const num = parseFloat(parts[0] ?? "");
244241
const den = parseFloat(parts[1] ?? "");
245-
if (den !== 0) return Math.round((num / den) * 100) / 100;
242+
if (Number.isFinite(num) && Number.isFinite(den) && den !== 0) {
243+
return Math.round((num / den) * 100) / 100;
244+
}
245+
return 0;
246246
}
247-
return parseFloat(frameRateStr) || 0;
247+
const parsed = parseFloat(frameRateStr);
248+
return Number.isFinite(parsed) ? parsed : 0;
248249
}
249250

250251
/**
@@ -270,6 +271,7 @@ export async function extractMediaMetadata(filePath: string): Promise<VideoMetad
270271
"json",
271272
"-show_format",
272273
"-show_streams",
274+
"--",
273275
filePath,
274276
]);
275277
output = parseProbeJson(stdout);
@@ -361,7 +363,7 @@ export async function extractAudioMetadata(
361363

362364
const probePromise = (async (): Promise<AudioMetadata> => {
363365
const stdout = await runFfprobe(
364-
["-v", "quiet", "-print_format", "json", "-show_format", "-show_streams", filePath],
366+
["-v", "quiet", "-print_format", "json", "-show_format", "-show_streams", "--", filePath],
365367
options?.signal,
366368
);
367369
const output = parseProbeJson(stdout);
@@ -383,6 +385,7 @@ export async function extractAudioMetadata(
383385
"stream=nb_read_packets",
384386
"-print_format",
385387
"json",
388+
"--",
386389
filePath,
387390
]);
388391
const packetOutput = parseProbeJson(packetStdout);
@@ -452,6 +455,7 @@ async function analyzeKeyframeIntervalsUncached(filePath: string): Promise<Keyfr
452455
"frame=pts_time",
453456
"-of",
454457
"csv=p=0",
458+
"--",
455459
filePath,
456460
]);
457461

0 commit comments

Comments
 (0)