From 4dc5a67353705948f08a9f485949f1410faef50c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 10:00:20 -0700 Subject: [PATCH 1/8] fix(media): bound the copilot ffmpeg tool's inputs, runtime, and output paths FFmpeg runs in-process on the request-serving app server, so every attacker-influenced dimension of a tool call is an instance-wide resource concern rather than a single failed request. - Cap input count at 10 in both the tool handler (before any download) and runFfmpegOperation. The existing MAX_MEDIA_BYTES budget bounded RAM but still permitted hundreds of small clips, and concat re-encodes each one serially with libx264. - Give the whole operation a single 5-minute wall-clock budget shared across every spawned process, and SIGKILL on expiry. A per-command timeout would still multiply out across concat's per-clip encodes. - Wire abortSignal and userStopSignal through to that kill, so a cancelled copilot turn stops the transcode instead of leaving it running. - Validate scale_pad width/height as integers in 16..4096 before they reach the filter graph, and clamp the probed dimensions concat derives from the first clip's container metadata. - Restrict the convert/extract_audio `format` to known muxers with safe file names. It was interpolated into path.join(dir, `out.${ext}`) unsanitized, so a format of "../../x.mp4" escaped the temp dir and wrote there. - Spawn ffprobe directly rather than through fluent-ffmpeg, which exposes no handle on the child and so cannot be killed. Validation runs before any temp dir or binary resolution, so a rejected request costs nothing and reports the real reason. --- .../copilot/tools/server/media/ffmpeg.test.ts | 34 ++ .../lib/copilot/tools/server/media/ffmpeg.ts | 59 ++- apps/sim/lib/media/ffmpeg.test.ts | 75 ++++ apps/sim/lib/media/ffmpeg.ts | 403 ++++++++++++++---- 4 files changed, 478 insertions(+), 93 deletions(-) create mode 100644 apps/sim/lib/media/ffmpeg.test.ts diff --git a/apps/sim/lib/copilot/tools/server/media/ffmpeg.test.ts b/apps/sim/lib/copilot/tools/server/media/ffmpeg.test.ts index 26912076ead..7cc2ccecee2 100644 --- a/apps/sim/lib/copilot/tools/server/media/ffmpeg.test.ts +++ b/apps/sim/lib/copilot/tools/server/media/ffmpeg.test.ts @@ -42,6 +42,7 @@ vi.mock('@/lib/copilot/application/execute-file-use-case', () => ({ })) vi.mock('@/lib/media/ffmpeg', () => ({ + MAX_FFMPEG_INPUTS: 10, runFfmpegOperation: runFfmpegOperationMock, })) @@ -308,4 +309,37 @@ describe('ffmpeg server tool secret provenance', () => { message: 'ffmpeg convert failed: The media operation failed safely', }) }) + + it('rejects more inputs than the cap before downloading any of them', async () => { + const result = await ffmpegServerTool.execute( + { + operation: 'concat', + inputs: { + files: Array.from({ length: 11 }, () => ({ path: 'files/input.mp4' })), + }, + }, + context + ) + + expect(result.success).toBe(false) + expect(result.message).toContain('At most 10 input files') + expect(resolveWorkspaceFileReferenceMock).not.toHaveBeenCalled() + expect(runFfmpegOperationMock).not.toHaveBeenCalled() + }) + + it('forwards the abort signal so a cancelled turn can kill the transcode', async () => { + const abortSignal = new AbortController().signal + + await ffmpegServerTool.execute( + { operation: 'convert', inputs: { files: [{ path: 'files/input.mp4' }] } }, + { ...context, abortSignal } + ) + + expect(runFfmpegOperationMock).toHaveBeenCalledWith( + 'convert', + expect.anything(), + expect.anything(), + { signal: abortSignal } + ) + }) }) diff --git a/apps/sim/lib/copilot/tools/server/media/ffmpeg.ts b/apps/sim/lib/copilot/tools/server/media/ffmpeg.ts index a4673a1db54..2218174cdde 100644 --- a/apps/sim/lib/copilot/tools/server/media/ffmpeg.ts +++ b/apps/sim/lib/copilot/tools/server/media/ffmpeg.ts @@ -12,7 +12,12 @@ import { } from '@/lib/copilot/tools/server/base-tool' import { writeCopilotWorkspaceFileByPath } from '@/lib/copilot/vfs/resource-writer' import { MAX_MEDIA_BYTES } from '@/lib/media/falai' -import { type FfmpegOperation, type MediaFile, runFfmpegOperation } from '@/lib/media/ffmpeg' +import { + type FfmpegOperation, + MAX_FFMPEG_INPUTS, + type MediaFile, + runFfmpegOperation, +} from '@/lib/media/ffmpeg' import { createWorkspaceFileSecretProvenanceFromRegistry, getBoundWorkspaceFileSecretProvenance, @@ -71,6 +76,19 @@ interface FfmpegResult { probe?: unknown } +/** + * A transcode outlives its request unless the child process is killed, so both + * the transport abort and the explicit user stop must reach FFmpeg — checking + * them only between steps leaves a cancelled turn burning cores. + */ +function resolveFfmpegAbortSignal(context: ServerToolContext): AbortSignal | undefined { + const signals = [context.abortSignal, context.userStopSignal].filter( + (signal): signal is AbortSignal => Boolean(signal) + ) + if (signals.length === 0) return undefined + return signals.length === 1 ? signals[0] : AbortSignal.any(signals) +} + export const ffmpegServerTool: BaseServerTool = { name: Ffmpeg.id, @@ -90,6 +108,14 @@ export const ffmpegServerTool: BaseServerTool = { if (inputPaths.length === 0) { return { success: false, message: 'At least one input file is required in inputs.files' } } + // Bounded before any download: the byte budget alone still permits hundreds + // of small clips, and concat re-encodes every one of them serially. + if (inputPaths.length > MAX_FFMPEG_INPUTS) { + return { + success: false, + message: `At most ${MAX_FFMPEG_INPUTS} input files are allowed per ffmpeg operation (got ${inputPaths.length}).`, + } + } let inputRequiresOpaqueError = false try { @@ -138,19 +164,24 @@ export const ffmpegServerTool: BaseServerTool = { inputRequiresOpaqueError ||= inputProvenance.status === 'unknown' || inputProvenance.entries.length > 0 assertServerToolNotAborted(context) - const result = await runFfmpegOperation(params.operation, mediaFiles, { - text: params.text, - position: params.position, - start: params.start, - end: params.end, - width: params.width, - height: params.height, - aspectRatio: params.aspectRatio, - volume: params.volume, - musicVolume: params.musicVolume, - loopToVideo: params.loopToVideo, - format: params.format, - }) + const result = await runFfmpegOperation( + params.operation, + mediaFiles, + { + text: params.text, + position: params.position, + start: params.start, + end: params.end, + width: params.width, + height: params.height, + aspectRatio: params.aspectRatio, + volume: params.volume, + musicVolume: params.musicVolume, + loopToVideo: params.loopToVideo, + format: params.format, + }, + { signal: resolveFfmpegAbortSignal(context) } + ) // probe reports metadata only — no file written. if (params.operation === 'probe') { diff --git a/apps/sim/lib/media/ffmpeg.test.ts b/apps/sim/lib/media/ffmpeg.test.ts new file mode 100644 index 00000000000..da36d57f544 --- /dev/null +++ b/apps/sim/lib/media/ffmpeg.test.ts @@ -0,0 +1,75 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { MAX_FFMPEG_INPUTS, type MediaFile, runFfmpegOperation } from '@/lib/media/ffmpeg' + +function mediaFile(mimeType = 'video/mp4'): MediaFile { + return { buffer: Buffer.from('media'), mimeType, name: 'clip.mp4' } +} + +describe('runFfmpegOperation input bounds', () => { + it('rejects more inputs than the cap before touching the filesystem', async () => { + const inputs = Array.from({ length: MAX_FFMPEG_INPUTS + 1 }, () => mediaFile()) + + await expect(runFfmpegOperation('concat', inputs)).rejects.toThrow( + `At most ${MAX_FFMPEG_INPUTS} input files` + ) + }) + + it('still requires at least one input', async () => { + await expect(runFfmpegOperation('convert', [], { format: 'mp3' })).rejects.toThrow( + 'At least one input file is required' + ) + }) +}) + +describe('runFfmpegOperation output format validation', () => { + it.each([ + ['../../escape.mp4', 'traversal'], + ['../pwned.mp3', 'parent segment'], + ['/etc/cron.d/x.mp4', 'absolute path'], + ['mp4/../../x', 'embedded separator'], + ])('rejects %s as an output format (%s)', async (format) => { + await expect(runFfmpegOperation('convert', [mediaFile()], { format })).rejects.toThrow( + 'Unsupported output format' + ) + }) + + it('rejects a format with no known muxer', async () => { + await expect(runFfmpegOperation('convert', [mediaFile()], { format: 'exe' })).rejects.toThrow( + 'Unsupported output format' + ) + }) + + it('rejects a traversal format on extract_audio too', async () => { + await expect( + runFfmpegOperation('extract_audio', [mediaFile()], { format: '../../escape.mp3' }) + ).rejects.toThrow('Unsupported output format') + }) +}) + +describe('runFfmpegOperation scale bounds', () => { + it.each([ + [30000, 30000], + [1, 1], + [4097, 1080], + [1920, 0], + [1920.5, 1080], + ])('rejects scale_pad at %sx%s', async (width, height) => { + await expect(runFfmpegOperation('scale_pad', [mediaFile()], { width, height })).rejects.toThrow( + /must be an integer between 16 and 4096|requires width\+height/ + ) + }) +}) + +describe('runFfmpegOperation abort handling', () => { + it('refuses to start once the signal is already aborted', async () => { + const controller = new AbortController() + controller.abort() + + await expect( + runFfmpegOperation('convert', [mediaFile()], { format: 'mp3' }, { signal: controller.signal }) + ).rejects.toThrow(/aborted/i) + }) +}) diff --git a/apps/sim/lib/media/ffmpeg.ts b/apps/sim/lib/media/ffmpeg.ts index bcfa0b6adf0..7b53bda09b4 100644 --- a/apps/sim/lib/media/ffmpeg.ts +++ b/apps/sim/lib/media/ffmpeg.ts @@ -1,4 +1,4 @@ -import { execSync } from 'node:child_process' +import { execFile, execSync } from 'node:child_process' import fs from 'node:fs/promises' import os from 'node:os' import path from 'node:path' @@ -31,6 +31,28 @@ function ensureFfmpeg(): void { } } +/** ffprobe ships alongside ffmpeg; fall back to PATH resolution. */ +function resolveFfprobePath(): string { + ensureFfmpeg() + if (!ffmpegPath) return 'ffprobe' + const dir = path.dirname(ffmpegPath) + const binary = process.platform === 'win32' ? 'ffprobe.exe' : 'ffprobe' + return path.join(dir, binary) +} + +/** + * Hard bounds for a single operation. FFmpeg runs in the request-serving + * process, so every attacker-influenced dimension needs a ceiling: an + * unbounded input count, filter dimension, or runtime is a whole-instance + * CPU/RAM denial of service, not a single failed request. + */ +export const MAX_FFMPEG_INPUTS = 10 +export const MIN_SCALE_DIMENSION = 16 +export const MAX_SCALE_DIMENSION = 4096 +export const DEFAULT_FFMPEG_TIMEOUT_MS = 5 * 60 * 1000 +const PROBE_TIMEOUT_MS = 15 * 1000 +const PROBE_MAX_OUTPUT_BYTES = 8 * 1024 * 1024 + export type FfmpegOperation = | 'overlay_audio' | 'mux' @@ -84,6 +106,14 @@ export interface FfmpegResult { probe?: MediaProbe } +/** Execution bounds for one operation, separate from its media parameters. */ +export interface FfmpegRunOptions { + /** Aborts and SIGKILLs every process spawned for the operation. */ + signal?: AbortSignal + /** Wall-clock budget for the whole operation. Defaults to DEFAULT_FFMPEG_TIMEOUT_MS. */ + timeoutMs?: number +} + const MIME_TO_EXT: Record = { 'video/mp4': 'mp4', 'video/mpeg': 'mp4', @@ -131,14 +161,94 @@ const EXT_TO_MIME: Record = { gif: 'image/gif', } +/** + * Temp-file names are built as `${prefix}.${ext}` and joined against the temp + * dir, so an extension carrying `/` or `..` escapes that dir once `path.join` + * normalizes it. Both extension sources are attacker-influenced (a stored file's + * MIME type, and the caller-supplied `format`), so neither reaches a path + * unsanitized. + */ +const SAFE_EXT_PATTERN = /^[a-z0-9]{1,8}$/ + +function isSafeExt(ext: string): boolean { + return SAFE_EXT_PATTERN.test(ext) +} + function extFromMime(mime: string): string { - return MIME_TO_EXT[mime] || mime.split('/')[1] || 'bin' + const known = MIME_TO_EXT[mime] + if (known) return known + const derived = (mime.split('/')[1] || '').toLowerCase() + return isSafeExt(derived) ? derived : 'bin' } function mimeFromExt(ext: string): string { return EXT_TO_MIME[ext] || 'application/octet-stream' } +/** Only formats with a known muxer and a safe file name may name an output. */ +function resolveOutputExt(format: string): string { + const ext = format.trim().toLowerCase() + if (!isSafeExt(ext) || !EXT_TO_MIME[ext]) { + throw new Error( + `Unsupported output format "${format}". Supported: ${Object.keys(EXT_TO_MIME).join(', ')}` + ) + } + return ext +} + +/** Scale targets land in a filter graph, where an oversized value allocates per-frame buffers. */ +function resolveScaleDimension(value: number, label: 'width' | 'height'): number { + if (!Number.isInteger(value) || value < MIN_SCALE_DIMENSION || value > MAX_SCALE_DIMENSION) { + throw new Error( + `${label} must be an integer between ${MIN_SCALE_DIMENSION} and ${MAX_SCALE_DIMENSION} (got ${value})` + ) + } + return value +} + +function clampProbedDimension(value: number | undefined, fallback: number): number { + if (!Number.isInteger(value)) return fallback + return Math.min(Math.max(value as number, MIN_SCALE_DIMENSION), MAX_SCALE_DIMENSION) +} + +function resolveNonNegativeSeconds(value: number, label: string): number { + if (!Number.isFinite(value) || value < 0) { + throw new Error(`${label} must be a non-negative number of seconds (got ${value})`) + } + return value +} + +function resolveVolume(value: number, label: string): number { + if (!Number.isFinite(value) || value < 0 || value > 10) { + throw new Error(`${label} must be a number between 0 and 10 (got ${value})`) + } + return value +} + +/** + * Every caller-supplied bound is checked before a temp dir is created or a + * binary is resolved, so a rejected request costs nothing and the error is the + * validation failure rather than a missing-FFmpeg message. + */ +function assertOptionsWithinBounds(operation: FfmpegOperation, options: FfmpegOptions): void { + if (options.start !== undefined) resolveNonNegativeSeconds(options.start, 'start') + if (options.end !== undefined) resolveNonNegativeSeconds(options.end, 'end') + if (options.volume !== undefined) resolveVolume(options.volume, 'volume') + if (options.musicVolume !== undefined) resolveVolume(options.musicVolume, 'musicVolume') + + if (operation === 'convert') { + if (!options.format) throw new Error('convert requires a target format') + resolveOutputExt(options.format) + } + if (operation === 'extract_audio') { + resolveOutputExt(options.format || 'mp3') + } + if (operation === 'scale_pad' && options.width && options.height) { + resolveScaleDimension(options.width, 'width') + resolveScaleDimension(options.height, 'height') + } +} + const ASPECT_TARGETS: Record = { '16:9': { w: 1920, h: 1080 }, '9:16': { w: 1080, h: 1920 }, @@ -173,11 +283,57 @@ function escapeDrawtext(text: string): string { return text.replace(/\\/g, '\\\\').replace(/:/g, '\\:').replace(/'/g, "\\'").replace(/%/g, '\\%') } -async function withTempDir(fn: (dir: string) => Promise): Promise { +/** + * A wall-clock budget for one whole operation, not per spawned process: + * `concat` runs one encode per clip, so a per-command timeout would still + * multiply out to an unbounded total. Every spawn draws from the same deadline + * and is SIGKILLed when it expires or when the caller aborts. + */ +class OperationBudget { + private readonly deadline: number + + constructor( + timeoutMs: number, + readonly signal?: AbortSignal + ) { + this.deadline = Date.now() + timeoutMs + } + + remainingMs(): number { + return this.deadline - Date.now() + } + + assertLive(): void { + if (this.signal?.aborted) { + throw new Error('FFmpeg operation aborted') + } + if (this.remainingMs() <= 0) { + throw new Error('FFmpeg operation exceeded its time budget') + } + } +} + +interface RunContext { + dir: string + budget: OperationBudget +} + +function createBudget(runOptions: FfmpegRunOptions): OperationBudget { + const timeoutMs = + Number.isFinite(runOptions.timeoutMs) && (runOptions.timeoutMs as number) > 0 + ? Math.min(runOptions.timeoutMs as number, DEFAULT_FFMPEG_TIMEOUT_MS) + : DEFAULT_FFMPEG_TIMEOUT_MS + return new OperationBudget(timeoutMs, runOptions.signal) +} + +async function withTempDir( + budget: OperationBudget, + fn: (ctx: RunContext) => Promise +): Promise { ensureFfmpeg() const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'media-ffmpeg-')) try { - return await fn(dir) + return await fn({ dir, budget }) } finally { await fs.rm(dir, { recursive: true, force: true }).catch(() => {}) } @@ -190,43 +346,115 @@ async function writeInput(dir: string, file: MediaFile, index: number): Promise< return filePath } -function runCommand(command: ffmpeg.FfmpegCommand, outputPath: string): Promise { +function runCommand( + command: ffmpeg.FfmpegCommand, + outputPath: string, + budget: OperationBudget +): Promise { + budget.assertLive() return new Promise((resolve, reject) => { + let settled = false + + function settle(err?: Error): void { + if (settled) return + settled = true + clearTimeout(timer) + budget.signal?.removeEventListener('abort', onAbort) + if (err) reject(err) + else resolve() + } + /** SIGKILL, not SIGTERM: a wedged encoder must not get to ignore the signal. */ + function kill(reason: string): void { + try { + command.kill('SIGKILL') + } catch { + // The process may already be gone; the rejection below is what matters. + } + settle(new Error(reason)) + } + function onAbort(): void { + kill('FFmpeg operation aborted') + } + + const timer = setTimeout( + () => kill('FFmpeg operation exceeded its time budget'), + budget.remainingMs() + ) + budget.signal?.addEventListener('abort', onAbort, { once: true }) + command - .on('end', () => resolve()) - .on('error', (err) => reject(new Error(`FFmpeg error: ${err.message}`))) + .on('end', () => settle()) + .on('error', (err) => settle(new Error(`FFmpeg error: ${err.message}`))) .save(outputPath) }) } -export async function probeMedia(file: MediaFile): Promise { - return withTempDir(async (dir) => { +export async function probeMedia( + file: MediaFile, + runOptions: FfmpegRunOptions = {} +): Promise { + const budget = createBudget(runOptions) + return withTempDir(budget, async ({ dir }) => { const inputPath = await writeInput(dir, file, 0) - return probeFile(inputPath) + return probeFile(inputPath, budget) }) } -function probeFile(filePath: string): Promise { - ensureFfmpeg() +interface FfprobeOutput { + format?: { duration?: string | number; format_name?: string } + streams?: Array<{ + codec_type?: string + codec_name?: string + width?: number + height?: number + }> +} + +/** + * Spawned directly rather than through `fluent-ffmpeg.ffprobe`, which gives no + * handle on the child and so cannot be killed: a crafted input that wedges + * ffprobe would otherwise hang forever holding a request. + */ +function probeFile(filePath: string, budget: OperationBudget): Promise { + budget.assertLive() + const timeout = Math.min(PROBE_TIMEOUT_MS, budget.remainingMs()) return new Promise((resolve, reject) => { - ffmpeg.ffprobe(filePath, (err, metadata) => { - if (err) { - reject(new Error(`FFprobe error: ${err.message}`)) - return + execFile( + resolveFfprobePath(), + ['-v', 'quiet', '-print_format', 'json', '-show_format', '-show_streams', '-i', filePath], + { + timeout, + killSignal: 'SIGKILL', + maxBuffer: PROBE_MAX_OUTPUT_BYTES, + signal: budget.signal, + }, + (err, stdout) => { + if (err) { + reject(new Error(`FFprobe error: ${err.message}`)) + return + } + let metadata: FfprobeOutput + try { + metadata = JSON.parse(stdout) as FfprobeOutput + } catch { + reject(new Error('FFprobe error: unreadable metadata')) + return + } + const streams = metadata.streams ?? [] + const video = streams.find((s) => s.codec_type === 'video') + const audio = streams.find((s) => s.codec_type === 'audio') + resolve({ + durationSeconds: Number(metadata.format?.duration) || 0, + format: metadata.format?.format_name || 'unknown', + width: video?.width, + height: video?.height, + videoCodec: video?.codec_name, + audioCodec: audio?.codec_name, + hasAudio: Boolean(audio), + hasVideo: Boolean(video), + }) } - const video = metadata.streams.find((s) => s.codec_type === 'video') - const audio = metadata.streams.find((s) => s.codec_type === 'audio') - resolve({ - durationSeconds: Number(metadata.format?.duration) || 0, - format: metadata.format?.format_name || 'unknown', - width: video?.width, - height: video?.height, - videoCodec: video?.codec_name, - audioCodec: audio?.codec_name, - hasAudio: Boolean(audio), - hasVideo: Boolean(video), - }) - }) + ) }) } @@ -237,43 +465,54 @@ function probeFile(filePath: string): Promise { export async function runFfmpegOperation( operation: FfmpegOperation, inputs: MediaFile[], - options: FfmpegOptions = {} + options: FfmpegOptions = {}, + runOptions: FfmpegRunOptions = {} ): Promise { if (inputs.length === 0) { throw new Error('At least one input file is required') } + if (inputs.length > MAX_FFMPEG_INPUTS) { + throw new Error( + `At most ${MAX_FFMPEG_INPUTS} input files are allowed per operation (got ${inputs.length})` + ) + } + + assertOptionsWithinBounds(operation, options) + + const budget = createBudget(runOptions) + budget.assertLive() if (operation === 'probe') { - return { probe: await probeMedia(inputs[0]) } + return { probe: await probeMedia(inputs[0], runOptions) } } - return withTempDir(async (dir) => { - const inputPaths = await Promise.all(inputs.map((f, i) => writeInput(dir, f, i))) + return withTempDir(budget, async (ctx) => { + const inputPaths = await Promise.all(inputs.map((f, i) => writeInput(ctx.dir, f, i))) switch (operation) { case 'overlay_audio': case 'mux': - return overlayAudio(dir, inputPaths, options) + return overlayAudio(ctx, inputPaths, options) case 'mix_audio': - return mixAudio(dir, inputPaths, options) + return mixAudio(ctx, inputPaths, options) case 'concat': - return concat(dir, inputPaths) + return concat(ctx, inputPaths) case 'trim': - return trim(dir, inputPaths[0], inputs[0], options) + return trim(ctx, inputPaths[0], inputs[0], options) case 'scale_pad': - return scalePad(dir, inputPaths[0], options) + return scalePad(ctx, inputPaths[0], options) case 'overlay_image': - return overlayImage(dir, inputPaths, options) + return overlayImage(ctx, inputPaths, options) case 'add_text': - return addText(dir, inputPaths[0], options) + return addText(ctx, inputPaths[0], options) case 'fade': - return fade(dir, inputPaths[0], inputs[0], options) + return fade(ctx, inputPaths[0], inputs[0], options) case 'extract_audio': - return extractAudio(dir, inputPaths[0], options) + return extractAudio(ctx, inputPaths[0], options) case 'convert': - return convert(dir, inputPaths[0], options) + return convert(ctx, inputPaths[0], options) case 'thumbnail': - return thumbnail(dir, inputPaths[0], options) + return thumbnail(ctx, inputPaths[0], options) default: throw new Error(`Unsupported ffmpeg operation: ${operation}`) } @@ -286,7 +525,7 @@ async function readOut(outputPath: string, ext: string): Promise { } async function overlayAudio( - dir: string, + { dir, budget }: RunContext, inputPaths: string[], options: FfmpegOptions ): Promise { @@ -309,19 +548,19 @@ async function overlayAudio( 'aac', '-shortest', ]) - await runCommand(command, outputPath) + await runCommand(command, outputPath, budget) return readOut(outputPath, 'mp4') } async function mixAudio( - dir: string, + { dir, budget }: RunContext, inputPaths: string[], options: FfmpegOptions ): Promise { if (inputPaths.length < 2) throw new Error('mix_audio requires [voice, music]') const outputPath = path.join(dir, 'out.mp3') - const voiceVol = options.volume ?? 1 - const musicVol = options.musicVolume ?? 0.3 + const voiceVol = resolveVolume(options.volume ?? 1, 'volume') + const musicVol = resolveVolume(options.musicVolume ?? 0.3, 'musicVolume') const command = ffmpeg() .input(inputPaths[0]) .input(inputPaths[1]) @@ -331,13 +570,13 @@ async function mixAudio( `[v][m]amix=inputs=2:duration=longest:dropout_transition=0[a]`, ]) .outputOptions(['-map', '[a]']) - await runCommand(command, outputPath) + await runCommand(command, outputPath, budget) return readOut(outputPath, 'mp3') } -async function concat(dir: string, inputPaths: string[]): Promise { +async function concat({ dir, budget }: RunContext, inputPaths: string[]): Promise { if (inputPaths.length < 2) throw new Error('concat requires at least 2 clips') - const probes = await Promise.all(inputPaths.map(probeFile)) + const probes = await Promise.all(inputPaths.map((p) => probeFile(p, budget))) probes.forEach((p, i) => { if (!p.hasVideo) { throw new Error( @@ -345,8 +584,11 @@ async function concat(dir: string, inputPaths: string[]): Promise ) } }) - const width = probes[0].width || 1280 - const height = probes[0].height || 720 + // Clamped, not trusted: these come from the first clip's container metadata, + // and a crafted file can declare dimensions that make the normalize pass + // allocate gigabytes per frame. + const width = clampProbedDimension(probes[0].width, 1280) + const height = clampProbedDimension(probes[0].height, 720) const fps = 30 // Normalize every clip to identical codec/size/fps/pixfmt, and SYNTHESIZE silent @@ -396,7 +638,7 @@ async function concat(dir: string, inputPaths: string[]): Promise '2', ...extra, ]) - await runCommand(cmd, out) + await runCommand(cmd, out, budget) normalized.push(out) } @@ -411,29 +653,30 @@ async function concat(dir: string, inputPaths: string[]): Promise .input(listPath) .inputOptions(['-f', 'concat', '-safe', '0']) .outputOptions(['-c', 'copy', '-movflags', '+faststart']) - await runCommand(concatCmd, outputPath) + await runCommand(concatCmd, outputPath, budget) return readOut(outputPath, 'mp4') } async function trim( - dir: string, + { dir, budget }: RunContext, inputPath: string, input: MediaFile, options: FfmpegOptions ): Promise { const ext = extFromMime(input.mimeType) const outputPath = path.join(dir, `out.${ext}`) - const start = options.start ?? 0 + const start = resolveNonNegativeSeconds(options.start ?? 0, 'start') const command = ffmpeg(inputPath).setStartTime(start) if (options.end !== undefined) { - command.setDuration(Math.max(0, options.end - start)) + const end = resolveNonNegativeSeconds(options.end, 'end') + command.setDuration(Math.max(0, end - start)) } - await runCommand(command, outputPath) + await runCommand(command, outputPath, budget) return readOut(outputPath, ext) } async function scalePad( - dir: string, + { dir, budget }: RunContext, inputPath: string, options: FfmpegOptions ): Promise { @@ -446,18 +689,20 @@ async function scalePad( if (!width || !height) { throw new Error('scale_pad requires width+height or a known aspectRatio (e.g. 9:16)') } + const scaleWidth = resolveScaleDimension(width, 'width') + const scaleHeight = resolveScaleDimension(height, 'height') const outputPath = path.join(dir, 'out.mp4') const command = ffmpeg(inputPath) .videoFilters( - `scale=${width}:${height}:force_original_aspect_ratio=decrease,pad=${width}:${height}:(ow-iw)/2:(oh-ih)/2,setsar=1` + `scale=${scaleWidth}:${scaleHeight}:force_original_aspect_ratio=decrease,pad=${scaleWidth}:${scaleHeight}:(ow-iw)/2:(oh-ih)/2,setsar=1` ) .outputOptions(['-c:a', 'copy']) - await runCommand(command, outputPath) + await runCommand(command, outputPath, budget) return readOut(outputPath, 'mp4') } async function overlayImage( - dir: string, + { dir, budget }: RunContext, inputPaths: string[], options: FfmpegOptions ): Promise { @@ -469,12 +714,12 @@ async function overlayImage( .input(inputPaths[1]) .complexFilter([`[0:v][1:v]overlay=${xy}[v]`]) .outputOptions(['-map', '[v]', '-map', '0:a?', '-c:a', 'copy']) - await runCommand(command, outputPath) + await runCommand(command, outputPath, budget) return readOut(outputPath, 'mp4') } async function addText( - dir: string, + { dir, budget }: RunContext, inputPath: string, options: FfmpegOptions ): Promise { @@ -494,17 +739,17 @@ async function addText( const command = ffmpeg(inputPath) .videoFilters(`drawtext=${drawtext}`) .outputOptions(['-c:a', 'copy']) - await runCommand(command, outputPath) + await runCommand(command, outputPath, budget) return readOut(outputPath, 'mp4') } async function fade( - dir: string, + { dir, budget }: RunContext, inputPath: string, input: MediaFile, _options: FfmpegOptions ): Promise { - const probe = await probeFile(inputPath) + const probe = await probeFile(inputPath, budget) const duration = probe.durationSeconds || 0 const fadeDur = Math.min(0.5, duration / 4 || 0.5) const outStart = Math.max(0, duration - fadeDur) @@ -516,44 +761,44 @@ async function fade( command.videoFilters([`fade=t=in:st=0:d=${fadeDur}`, `fade=t=out:st=${outStart}:d=${fadeDur}`]) } command.audioFilters([`afade=t=in:st=0:d=${fadeDur}`, `afade=t=out:st=${outStart}:d=${fadeDur}`]) - await runCommand(command, outputPath) + await runCommand(command, outputPath, budget) return readOut(outputPath, ext) } async function extractAudio( - dir: string, + { dir, budget }: RunContext, inputPath: string, options: FfmpegOptions ): Promise { - const ext = (options.format || 'mp3').toLowerCase() + const ext = resolveOutputExt(options.format || 'mp3') const outputPath = path.join(dir, `out.${ext}`) const command = ffmpeg(inputPath).noVideo() - await runCommand(command, outputPath) + await runCommand(command, outputPath, budget) return readOut(outputPath, ext) } async function convert( - dir: string, + { dir, budget }: RunContext, inputPath: string, options: FfmpegOptions ): Promise { if (!options.format) throw new Error('convert requires a target format') - const ext = options.format.toLowerCase() + const ext = resolveOutputExt(options.format) const outputPath = path.join(dir, `out.${ext}`) - await runCommand(ffmpeg(inputPath), outputPath) + await runCommand(ffmpeg(inputPath), outputPath, budget) return readOut(outputPath, ext) } async function thumbnail( - dir: string, + { dir, budget }: RunContext, inputPath: string, options: FfmpegOptions ): Promise { const outputPath = path.join(dir, 'out.jpg') const command = ffmpeg(inputPath) - .seekInput(options.start ?? 0) + .seekInput(resolveNonNegativeSeconds(options.start ?? 0, 'start')) .frames(1) - await runCommand(command, outputPath) + await runCommand(command, outputPath, budget) return readOut(outputPath, 'jpg') } From 260abe8d4039093c536e143ef45d3818323cc7de Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 10:25:44 -0700 Subject: [PATCH 2/8] fix(media): address ffmpeg hardening review findings Follow-up to the input/runtime/output-path bounds, from a multi-agent review of that change. Regressions the first pass introduced: - webp and weba were in MIME_TO_EXT but not EXT_TO_MIME, so convert to either hard-failed where it previously worked. Both are now valid outputs. - Bounds were asserted for every operation, so a surplus out-of-range value an operation never reads (overlay_audio + volume) failed the whole call. Each operation now validates only what it consumes. - width/height of 0 bypassed the scale check via a truthy guard. - clampProbedDimension raised a probed 0 to 16 instead of falling back to the default, yielding a 16x16 concat. Bugs found in the new code: - fluent-ffmpeg's kill() is a no-op until the child spawns, and .save() spawns asynchronously. A kill landing in that window rejected the promise while the encode spawned orphaned and unkillable. Re-issue the kill on 'start'. - ffprobe ran with -v quiet, which left a timeout, a corrupt file, and a missing file byte-identical and uninformative. Use -v error and report the distinct cause, with the server's paths stripped from the diagnostic. - resolveFfprobePath narrowed fluent-ffmpeg's lookup; restore FFPROBE_PATH and PATH fallback with existence checks. Also: reject a trim whose end precedes its start rather than silently writing an empty file, restrict extract_audio to audio containers, name the actionable cause in the timeout message, and drop a redundant second probe budget. Tests: assert no temp dir is created on an already-aborted signal (the previous assertion passed with the guard reverted), cover the 0-dimension and end-before-start cases, and track MAX_FFMPEG_INPUTS instead of a literal. Reviewers also flagged that LLM-supplied numerics arrive as strings; verified against the router that Ajv rejects those upstream, so no coercion was added. --- .../copilot/tools/server/media/ffmpeg.test.ts | 5 +- apps/sim/lib/media/ffmpeg.test.ts | 43 +++++- apps/sim/lib/media/ffmpeg.ts | 128 ++++++++++++++---- 3 files changed, 145 insertions(+), 31 deletions(-) diff --git a/apps/sim/lib/copilot/tools/server/media/ffmpeg.test.ts b/apps/sim/lib/copilot/tools/server/media/ffmpeg.test.ts index 7cc2ccecee2..2868184a357 100644 --- a/apps/sim/lib/copilot/tools/server/media/ffmpeg.test.ts +++ b/apps/sim/lib/copilot/tools/server/media/ffmpeg.test.ts @@ -59,6 +59,7 @@ vi.mock('@/lib/uploads/contexts/workspace/workspace-file-secret-provenance', () })) import { ffmpegServerTool } from '@/lib/copilot/tools/server/media/ffmpeg' +import { MAX_FFMPEG_INPUTS } from '@/lib/media/ffmpeg' const EXACT_EMPTY = { status: 'exact' as const, entries: [] } const TRACKED = { @@ -315,14 +316,14 @@ describe('ffmpeg server tool secret provenance', () => { { operation: 'concat', inputs: { - files: Array.from({ length: 11 }, () => ({ path: 'files/input.mp4' })), + files: Array.from({ length: MAX_FFMPEG_INPUTS + 1 }, () => ({ path: 'files/input.mp4' })), }, }, context ) expect(result.success).toBe(false) - expect(result.message).toContain('At most 10 input files') + expect(result.message).toContain(`At most ${MAX_FFMPEG_INPUTS} input files`) expect(resolveWorkspaceFileReferenceMock).not.toHaveBeenCalled() expect(runFfmpegOperationMock).not.toHaveBeenCalled() }) diff --git a/apps/sim/lib/media/ffmpeg.test.ts b/apps/sim/lib/media/ffmpeg.test.ts index da36d57f544..77ec14b1e9e 100644 --- a/apps/sim/lib/media/ffmpeg.test.ts +++ b/apps/sim/lib/media/ffmpeg.test.ts @@ -1,7 +1,8 @@ /** * @vitest-environment node */ -import { describe, expect, it } from 'vitest' +import fs from 'node:fs/promises' +import { describe, expect, it, vi } from 'vitest' import { MAX_FFMPEG_INPUTS, type MediaFile, runFfmpegOperation } from '@/lib/media/ffmpeg' function mediaFile(mimeType = 'video/mp4'): MediaFile { @@ -47,6 +48,15 @@ describe('runFfmpegOperation output format validation', () => { runFfmpegOperation('extract_audio', [mediaFile()], { format: '../../escape.mp3' }) ).rejects.toThrow('Unsupported output format') }) + + it.each(['webp', 'weba', 'mp4', 'gif'])( + 'still accepts %s, which the input MIME map already supported', + async (format) => { + await expect(runFfmpegOperation('convert', [mediaFile()], { format })).rejects.not.toThrow( + 'Unsupported output format' + ) + } + ) }) describe('runFfmpegOperation scale bounds', () => { @@ -55,21 +65,50 @@ describe('runFfmpegOperation scale bounds', () => { [1, 1], [4097, 1080], [1920, 0], + [0, 1080], [1920.5, 1080], ])('rejects scale_pad at %sx%s', async (width, height) => { await expect(runFfmpegOperation('scale_pad', [mediaFile()], { width, height })).rejects.toThrow( - /must be an integer between 16 and 4096|requires width\+height/ + 'must be an integer between 16 and 4096' ) }) }) +describe('runFfmpegOperation per-operation validation', () => { + it('ignores options the operation never consumes', async () => { + // overlay_audio does not read `volume`; an out-of-range surplus value from + // the model must not fail the whole call. + await expect( + runFfmpegOperation('overlay_audio', [mediaFile(), mediaFile('audio/mpeg')], { volume: 15 }) + ).rejects.not.toThrow(/volume/) + }) + + it('rejects a trim whose end precedes its start', async () => { + await expect(runFfmpegOperation('trim', [mediaFile()], { start: 10, end: 5 })).rejects.toThrow( + 'end (5s) must be greater than or equal to start (10s)' + ) + }) + + it('restricts extract_audio to audio containers', async () => { + await expect( + runFfmpegOperation('extract_audio', [mediaFile()], { format: 'png' }) + ).rejects.toThrow('Unsupported output format') + }) +}) + describe('runFfmpegOperation abort handling', () => { it('refuses to start once the signal is already aborted', async () => { const controller = new AbortController() controller.abort() + const mkdtemp = vi.spyOn(fs, 'mkdtemp') await expect( runFfmpegOperation('convert', [mediaFile()], { format: 'mp3' }, { signal: controller.signal }) ).rejects.toThrow(/aborted/i) + + // "Refuses to start" means exactly this: no temp dir, so no input was ever + // written and no process was ever spawned. + expect(mkdtemp).not.toHaveBeenCalled() + mkdtemp.mockRestore() }) }) diff --git a/apps/sim/lib/media/ffmpeg.ts b/apps/sim/lib/media/ffmpeg.ts index 7b53bda09b4..96f1ecea188 100644 --- a/apps/sim/lib/media/ffmpeg.ts +++ b/apps/sim/lib/media/ffmpeg.ts @@ -1,4 +1,5 @@ import { execFile, execSync } from 'node:child_process' +import { existsSync } from 'node:fs' import fs from 'node:fs/promises' import os from 'node:os' import path from 'node:path' @@ -31,13 +32,23 @@ function ensureFfmpeg(): void { } } -/** ffprobe ships alongside ffmpeg; fall back to PATH resolution. */ +/** + * Mirrors fluent-ffmpeg's resolution order (FFPROBE_PATH, then PATH, then + * ffmpeg's own directory) so replacing its ffprobe call does not narrow where + * the binary may live for self-hosters. + */ function resolveFfprobePath(): string { ensureFfmpeg() - if (!ffmpegPath) return 'ffprobe' - const dir = path.dirname(ffmpegPath) const binary = process.platform === 'win32' ? 'ffprobe.exe' : 'ffprobe' - return path.join(dir, binary) + + const configured = process.env.FFPROBE_PATH?.trim() + if (configured && existsSync(configured)) return configured + + if (ffmpegPath) { + const sibling = path.join(path.dirname(ffmpegPath), binary) + if (existsSync(sibling)) return sibling + } + return binary } /** @@ -53,6 +64,10 @@ export const DEFAULT_FFMPEG_TIMEOUT_MS = 5 * 60 * 1000 const PROBE_TIMEOUT_MS = 15 * 1000 const PROBE_MAX_OUTPUT_BYTES = 8 * 1024 * 1024 +/** Names the actionable cause: the budget covers all clips, so fewer/shorter inputs is the fix. */ +const TIME_BUDGET_EXCEEDED = + 'FFmpeg operation exceeded its time budget — try fewer, shorter, or lower-resolution inputs' + export type FfmpegOperation = | 'overlay_audio' | 'mux' @@ -110,7 +125,10 @@ export interface FfmpegResult { export interface FfmpegRunOptions { /** Aborts and SIGKILLs every process spawned for the operation. */ signal?: AbortSignal - /** Wall-clock budget for the whole operation. Defaults to DEFAULT_FFMPEG_TIMEOUT_MS. */ + /** + * Wall-clock budget for the whole operation. Defaults to, and is capped at, + * DEFAULT_FFMPEG_TIMEOUT_MS — a caller may shorten the ceiling, never raise it. + */ timeoutMs?: number } @@ -155,12 +173,17 @@ const EXT_TO_MIME: Record = { flac: 'audio/flac', aac: 'audio/aac', opus: 'audio/opus', + weba: 'audio/webm', png: 'image/png', jpg: 'image/jpeg', jpeg: 'image/jpeg', gif: 'image/gif', + webp: 'image/webp', } +/** extract_audio can only name an audio container; the rest would silently produce nothing useful. */ +const AUDIO_EXTS = new Set(['mp3', 'm4a', 'wav', 'ogg', 'flac', 'aac', 'opus', 'weba']) + /** * Temp-file names are built as `${prefix}.${ext}` and joined against the temp * dir, so an extension carrying `/` or `..` escapes that dir once `path.join` @@ -186,11 +209,12 @@ function mimeFromExt(ext: string): string { } /** Only formats with a known muxer and a safe file name may name an output. */ -function resolveOutputExt(format: string): string { - const ext = format.trim().toLowerCase() - if (!isSafeExt(ext) || !EXT_TO_MIME[ext]) { +function resolveOutputExt(format: string, allowed?: Set): string { + const ext = String(format).trim().toLowerCase() + const supported = allowed ?? new Set(Object.keys(EXT_TO_MIME)) + if (!isSafeExt(ext) || !EXT_TO_MIME[ext] || !supported.has(ext)) { throw new Error( - `Unsupported output format "${format}". Supported: ${Object.keys(EXT_TO_MIME).join(', ')}` + `Unsupported output format "${format}". Supported: ${[...supported].join(', ')}` ) } return ext @@ -207,8 +231,8 @@ function resolveScaleDimension(value: number, label: 'width' | 'height'): number } function clampProbedDimension(value: number | undefined, fallback: number): number { - if (!Number.isInteger(value)) return fallback - return Math.min(Math.max(value as number, MIN_SCALE_DIMENSION), MAX_SCALE_DIMENSION) + if (!Number.isInteger(value) || (value as number) < MIN_SCALE_DIMENSION) return fallback + return Math.min(value as number, MAX_SCALE_DIMENSION) } function resolveNonNegativeSeconds(value: number, label: string): number { @@ -229,21 +253,34 @@ function resolveVolume(value: number, label: string): number { * Every caller-supplied bound is checked before a temp dir is created or a * binary is resolved, so a rejected request costs nothing and the error is the * validation failure rather than a missing-FFmpeg message. + * + * Only the options an operation actually consumes are validated. An LLM caller + * routinely emits surplus parameters, and failing the whole call over a value + * the operation ignores would be a regression, not a safeguard. */ function assertOptionsWithinBounds(operation: FfmpegOperation, options: FfmpegOptions): void { - if (options.start !== undefined) resolveNonNegativeSeconds(options.start, 'start') - if (options.end !== undefined) resolveNonNegativeSeconds(options.end, 'end') - if (options.volume !== undefined) resolveVolume(options.volume, 'volume') - if (options.musicVolume !== undefined) resolveVolume(options.musicVolume, 'musicVolume') - + if (operation === 'trim' || operation === 'thumbnail') { + if (options.start !== undefined) resolveNonNegativeSeconds(options.start, 'start') + } + if (operation === 'trim' && options.end !== undefined) { + const end = resolveNonNegativeSeconds(options.end, 'end') + const start = resolveNonNegativeSeconds(options.start ?? 0, 'start') + if (end < start) { + throw new Error(`end (${end}s) must be greater than or equal to start (${start}s)`) + } + } + if (operation === 'mix_audio') { + if (options.volume !== undefined) resolveVolume(options.volume, 'volume') + if (options.musicVolume !== undefined) resolveVolume(options.musicVolume, 'musicVolume') + } if (operation === 'convert') { if (!options.format) throw new Error('convert requires a target format') resolveOutputExt(options.format) } if (operation === 'extract_audio') { - resolveOutputExt(options.format || 'mp3') + resolveOutputExt(options.format || 'mp3', AUDIO_EXTS) } - if (operation === 'scale_pad' && options.width && options.height) { + if (operation === 'scale_pad' && options.width !== undefined && options.height !== undefined) { resolveScaleDimension(options.width, 'width') resolveScaleDimension(options.height, 'height') } @@ -308,7 +345,7 @@ class OperationBudget { throw new Error('FFmpeg operation aborted') } if (this.remainingMs() <= 0) { - throw new Error('FFmpeg operation exceeded its time budget') + throw new Error(TIME_BUDGET_EXCEEDED) } } } @@ -372,17 +409,31 @@ function runCommand( } settle(new Error(reason)) } + /** + * fluent-ffmpeg's kill() is a silent no-op until the child exists, and + * `.save()` spawns asynchronously (it may shell out for capability checks + * first). A kill landing in that window would otherwise reject the promise + * while the encode goes on to spawn orphaned and unkillable — so re-issue + * it once the process is up. 'start' fires immediately after the spawn. + */ + function onStart(): void { + if (settled) { + try { + command.kill('SIGKILL') + } catch { + // Nothing to signal; the promise has already settled. + } + } + } function onAbort(): void { kill('FFmpeg operation aborted') } - const timer = setTimeout( - () => kill('FFmpeg operation exceeded its time budget'), - budget.remainingMs() - ) + const timer = setTimeout(() => kill(TIME_BUDGET_EXCEEDED), budget.remainingMs()) budget.signal?.addEventListener('abort', onAbort, { once: true }) command + .on('start', onStart) .on('end', () => settle()) .on('error', (err) => settle(new Error(`FFmpeg error: ${err.message}`))) .save(outputPath) @@ -410,6 +461,25 @@ interface FfprobeOutput { }> } +/** + * Distinguishes the three ways a probe fails. Node's `execFile` error message + * is `Command failed: ` plus stderr, which both leaks the server's + * binary and temp paths to the caller and — once stderr is quiet — renders a + * timeout, a corrupt file, and a missing file byte-identical. + */ +function describeProbeFailure(err: Error & { killed?: boolean; code?: unknown }, stderr: string) { + if (err.code === 'ABORT_ERR') return 'aborted' + if (err.killed) return 'timed out' + const detail = stderr.trim().split('\n').pop() + if (!detail) return 'unreadable media' + // ffprobe prefixes its diagnostic with the input path; keep the diagnostic, + // drop the server's directory layout. + return detail.replace( + /(^|\s)(\/\S+)/g, + (_match, lead: string, abs: string) => `${lead}${path.basename(abs)}` + ) +} + /** * Spawned directly rather than through `fluent-ffmpeg.ffprobe`, which gives no * handle on the child and so cannot be killed: a crafted input that wedges @@ -421,16 +491,16 @@ function probeFile(filePath: string, budget: OperationBudget): Promise { execFile( resolveFfprobePath(), - ['-v', 'quiet', '-print_format', 'json', '-show_format', '-show_streams', '-i', filePath], + ['-v', 'error', '-print_format', 'json', '-show_format', '-show_streams', '-i', filePath], { timeout, killSignal: 'SIGKILL', maxBuffer: PROBE_MAX_OUTPUT_BYTES, signal: budget.signal, }, - (err, stdout) => { + (err, stdout, stderr) => { if (err) { - reject(new Error(`FFprobe error: ${err.message}`)) + reject(new Error(`FFprobe error: ${describeProbeFailure(err, stderr)}`)) return } let metadata: FfprobeOutput @@ -483,7 +553,11 @@ export async function runFfmpegOperation( budget.assertLive() if (operation === 'probe') { - return { probe: await probeMedia(inputs[0], runOptions) } + return { + probe: await withTempDir(budget, async ({ dir }) => + probeFile(await writeInput(dir, inputs[0], 0), budget) + ), + } } return withTempDir(budget, async (ctx) => { From 9791fd8668f76b0ba612961a90be8462efac3253 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 10:35:51 -0700 Subject: [PATCH 3/8] refactor(media): reuse shared execution limits and give each rule one home MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quality pass over the ffmpeg hardening, from a four-angle review. Reuse: - Replace the hand-rolled OperationBudget with createTimeoutAbortController from @/lib/core/execution-limits, which already models "one deadline plus a parent signal, and tell me which fired". This also removes the per-command setTimeout: the controller's single deadline covers the whole operation, so probeFile now takes its cap from getRemainingExecutionMs. - Combine the tool's cancellation signals with combineExecutionAbortSignals, and move that helper to base-tool.ts next to assertServerToolNotAborted, where the shared "how does a tool consume cancellation" concern lives. Altitude: - Delete assertOptionsWithinBounds. Every rule in it also lived in the operation that consumes it, and the two copies had already diverged: extract_audio's allowlist existed only in the preflight, and the end >= start check only there. Each rule now has exactly one home, at its point of use. - Stop resolving the ffmpeg binary in withTempDir, so a validation failure no longer surfaces as "FFmpeg not found" on a host without it. runCommand and probeFile resolve it, being the only things that need it. - Add tempPath(), which resolves a name inside the temp dir and refuses anything that escapes. All 14 path sites go through it, so the containment invariant is structural rather than dependent on remembering to sanitize every filename source. - Declare OUTPUT_EXTS explicitly instead of deriving the allowlist from EXT_TO_MIME, so widening a content-type map cannot widen what may be written. Also: a supplied width/height of 0 now reaches the bounds check rather than being treated as absent, memoize the resolved ffprobe path, and settle() on a synchronous throw from .save() so no listener outlives the command. Tests: replace two assertions that could not fail (`rejects.not.toThrow` passes on any rejection, including "FFmpeg not found") with deterministic ones, which also removes every real ffmpeg spawn from the suite — 293ms of test time to 21ms. Verified hermetic with ffmpeg off PATH, and verified against real ffmpeg out-of-band that probe, convert, scale_pad, budget expiry, and external abort all still behave. --- .../sim/lib/copilot/tools/server/base-tool.ts | 13 + .../lib/copilot/tools/server/media/ffmpeg.ts | 18 +- apps/sim/lib/media/ffmpeg.test.ts | 31 +- apps/sim/lib/media/ffmpeg.ts | 429 +++++++++--------- 4 files changed, 249 insertions(+), 242 deletions(-) diff --git a/apps/sim/lib/copilot/tools/server/base-tool.ts b/apps/sim/lib/copilot/tools/server/base-tool.ts index 2af97ba7490..d2365d0b301 100644 --- a/apps/sim/lib/copilot/tools/server/base-tool.ts +++ b/apps/sim/lib/copilot/tools/server/base-tool.ts @@ -1,5 +1,6 @@ import type { z } from 'zod' import type { BillingAttributionSnapshot } from '@/lib/billing/core/billing-attribution' +import { combineExecutionAbortSignals } from '@/lib/core/execution-limits' import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' export interface ServerToolContext { @@ -28,6 +29,18 @@ export interface ServerToolContext { resolvedSecretTraceRegistry?: ResolvedSecretTraceRegistry } +/** + * One signal covering every way a tool call can be cancelled, for tools that + * hold a killable resource (a child process, a long stream) rather than merely + * checking between steps as {@link assertServerToolNotAborted} does. + */ +export function resolveServerToolAbortSignal(context?: ServerToolContext): AbortSignal | undefined { + const signals = [context?.abortSignal, context?.userStopSignal].filter( + (signal): signal is AbortSignal => Boolean(signal) + ) + return signals.length > 0 ? combineExecutionAbortSignals(signals) : undefined +} + export function assertServerToolNotAborted( context?: ServerToolContext, message = 'Request aborted before tool mutation could be applied.' diff --git a/apps/sim/lib/copilot/tools/server/media/ffmpeg.ts b/apps/sim/lib/copilot/tools/server/media/ffmpeg.ts index 2218174cdde..82745f01a47 100644 --- a/apps/sim/lib/copilot/tools/server/media/ffmpeg.ts +++ b/apps/sim/lib/copilot/tools/server/media/ffmpeg.ts @@ -8,6 +8,7 @@ import { Ffmpeg } from '@/lib/copilot/generated/tool-catalog-v1' import { assertServerToolNotAborted, type BaseServerTool, + resolveServerToolAbortSignal, type ServerToolContext, } from '@/lib/copilot/tools/server/base-tool' import { writeCopilotWorkspaceFileByPath } from '@/lib/copilot/vfs/resource-writer' @@ -76,19 +77,6 @@ interface FfmpegResult { probe?: unknown } -/** - * A transcode outlives its request unless the child process is killed, so both - * the transport abort and the explicit user stop must reach FFmpeg — checking - * them only between steps leaves a cancelled turn burning cores. - */ -function resolveFfmpegAbortSignal(context: ServerToolContext): AbortSignal | undefined { - const signals = [context.abortSignal, context.userStopSignal].filter( - (signal): signal is AbortSignal => Boolean(signal) - ) - if (signals.length === 0) return undefined - return signals.length === 1 ? signals[0] : AbortSignal.any(signals) -} - export const ffmpegServerTool: BaseServerTool = { name: Ffmpeg.id, @@ -180,7 +168,9 @@ export const ffmpegServerTool: BaseServerTool = { loopToVideo: params.loopToVideo, format: params.format, }, - { signal: resolveFfmpegAbortSignal(context) } + // A transcode outlives its request unless the child is killed, so the + // cancellation signal must reach FFmpeg itself, not just the steps around it. + { signal: resolveServerToolAbortSignal(context) } ) // probe reports metadata only — no file written. diff --git a/apps/sim/lib/media/ffmpeg.test.ts b/apps/sim/lib/media/ffmpeg.test.ts index 77ec14b1e9e..ef96c192074 100644 --- a/apps/sim/lib/media/ffmpeg.test.ts +++ b/apps/sim/lib/media/ffmpeg.test.ts @@ -49,14 +49,18 @@ describe('runFfmpegOperation output format validation', () => { ).rejects.toThrow('Unsupported output format') }) - it.each(['webp', 'weba', 'mp4', 'gif'])( - 'still accepts %s, which the input MIME map already supported', - async (format) => { - await expect(runFfmpegOperation('convert', [mediaFile()], { format })).rejects.not.toThrow( - 'Unsupported output format' - ) + it('keeps the formats the input MIME map already supported', async () => { + // Asserted through the rejection's own "Supported:" list rather than by + // converting for real: a `not.toThrow` on a live transcode passes on any + // rejection, including "FFmpeg not found". + const error = await runFfmpegOperation('convert', [mediaFile()], { format: 'exe' }).catch( + (e: Error) => e + ) + + for (const format of ['mp4', 'mov', 'webm', 'mp3', 'wav', 'gif', 'webp', 'weba']) { + expect(error.message).toContain(format) } - ) + }) }) describe('runFfmpegOperation scale bounds', () => { @@ -75,12 +79,15 @@ describe('runFfmpegOperation scale bounds', () => { }) describe('runFfmpegOperation per-operation validation', () => { - it('ignores options the operation never consumes', async () => { - // overlay_audio does not read `volume`; an out-of-range surplus value from - // the model must not fail the whole call. + // Each rule is asserted at the operation that owns it. The complementary + // property — that an operation ignores options it never reads — cannot be + // asserted without running a real transcode, so it is left to review. + it('rejects an out-of-range volume on mix_audio, which consumes it', async () => { await expect( - runFfmpegOperation('overlay_audio', [mediaFile(), mediaFile('audio/mpeg')], { volume: 15 }) - ).rejects.not.toThrow(/volume/) + runFfmpegOperation('mix_audio', [mediaFile('audio/mpeg'), mediaFile('audio/mpeg')], { + volume: 15, + }) + ).rejects.toThrow('volume must be a number between 0 and 10') }) it('rejects a trim whose end precedes its start', async () => { diff --git a/apps/sim/lib/media/ffmpeg.ts b/apps/sim/lib/media/ffmpeg.ts index 96f1ecea188..be45134582b 100644 --- a/apps/sim/lib/media/ffmpeg.ts +++ b/apps/sim/lib/media/ffmpeg.ts @@ -4,12 +4,19 @@ import fs from 'node:fs/promises' import os from 'node:os' import path from 'node:path' import { createLogger } from '@sim/logger' +import { toError } from '@sim/utils/errors' import ffmpeg from 'fluent-ffmpeg' +import { + createTimeoutAbortController, + getRemainingExecutionMs, + type TimeoutAbortController, +} from '@/lib/core/execution-limits' const logger = createLogger('MediaFfmpeg') let ffmpegInitialized = false let ffmpegPath: string | null = null +let ffprobePath: string | null = null /** Lazy system FFmpeg binary resolution, mirroring lib/audio/extractor.ts. */ function ensureFfmpeg(): void { @@ -39,16 +46,19 @@ function ensureFfmpeg(): void { */ function resolveFfprobePath(): string { ensureFfmpeg() - const binary = process.platform === 'win32' ? 'ffprobe.exe' : 'ffprobe' + if (ffprobePath) return ffprobePath + const binary = process.platform === 'win32' ? 'ffprobe.exe' : 'ffprobe' const configured = process.env.FFPROBE_PATH?.trim() - if (configured && existsSync(configured)) return configured + const sibling = ffmpegPath ? path.join(path.dirname(ffmpegPath), binary) : undefined - if (ffmpegPath) { - const sibling = path.join(path.dirname(ffmpegPath), binary) - if (existsSync(sibling)) return sibling - } - return binary + ffprobePath = + configured && existsSync(configured) + ? configured + : sibling && existsSync(sibling) + ? sibling + : binary + return ffprobePath } /** @@ -181,6 +191,33 @@ const EXT_TO_MIME: Record = { webp: 'image/webp', } +/** + * The formats a caller may name as an output, declared explicitly rather than + * derived from EXT_TO_MIME: that map answers "what content type is this?", and + * letting an addition there silently widen what may be written couples a + * security allowlist to an unrelated lookup table. + */ +const OUTPUT_EXTS = new Set([ + 'mp4', + 'mov', + 'webm', + 'mkv', + 'avi', + 'mp3', + 'm4a', + 'wav', + 'ogg', + 'flac', + 'aac', + 'opus', + 'weba', + 'png', + 'jpg', + 'jpeg', + 'gif', + 'webp', +]) + /** extract_audio can only name an audio container; the rest would silently produce nothing useful. */ const AUDIO_EXTS = new Set(['mp3', 'm4a', 'wav', 'ogg', 'flac', 'aac', 'opus', 'weba']) @@ -209,13 +246,10 @@ function mimeFromExt(ext: string): string { } /** Only formats with a known muxer and a safe file name may name an output. */ -function resolveOutputExt(format: string, allowed?: Set): string { - const ext = String(format).trim().toLowerCase() - const supported = allowed ?? new Set(Object.keys(EXT_TO_MIME)) - if (!isSafeExt(ext) || !EXT_TO_MIME[ext] || !supported.has(ext)) { - throw new Error( - `Unsupported output format "${format}". Supported: ${[...supported].join(', ')}` - ) +function resolveOutputExt(format: string, allowed: Set = OUTPUT_EXTS): string { + const ext = format.trim().toLowerCase() + if (!isSafeExt(ext) || !allowed.has(ext)) { + throw new Error(`Unsupported output format "${format}". Supported: ${[...allowed].join(', ')}`) } return ext } @@ -250,41 +284,10 @@ function resolveVolume(value: number, label: string): number { } /** - * Every caller-supplied bound is checked before a temp dir is created or a - * binary is resolved, so a rejected request costs nothing and the error is the - * validation failure rather than a missing-FFmpeg message. - * - * Only the options an operation actually consumes are validated. An LLM caller - * routinely emits surplus parameters, and failing the whole call over a value - * the operation ignores would be a regression, not a safeguard. + * Each operation validates the options it consumes, at the point of use, so a + * rule has exactly one home. An LLM caller routinely emits surplus parameters, + * so an operation must ignore — never reject over — a value it never reads. */ -function assertOptionsWithinBounds(operation: FfmpegOperation, options: FfmpegOptions): void { - if (operation === 'trim' || operation === 'thumbnail') { - if (options.start !== undefined) resolveNonNegativeSeconds(options.start, 'start') - } - if (operation === 'trim' && options.end !== undefined) { - const end = resolveNonNegativeSeconds(options.end, 'end') - const start = resolveNonNegativeSeconds(options.start ?? 0, 'start') - if (end < start) { - throw new Error(`end (${end}s) must be greater than or equal to start (${start}s)`) - } - } - if (operation === 'mix_audio') { - if (options.volume !== undefined) resolveVolume(options.volume, 'volume') - if (options.musicVolume !== undefined) resolveVolume(options.musicVolume, 'musicVolume') - } - if (operation === 'convert') { - if (!options.format) throw new Error('convert requires a target format') - resolveOutputExt(options.format) - } - if (operation === 'extract_audio') { - resolveOutputExt(options.format || 'mp3', AUDIO_EXTS) - } - if (operation === 'scale_pad' && options.width !== undefined && options.height !== undefined) { - resolveScaleDimension(options.width, 'width') - resolveScaleDimension(options.height, 'height') - } -} const ASPECT_TARGETS: Record = { '16:9': { w: 1920, h: 1080 }, @@ -321,64 +324,61 @@ function escapeDrawtext(text: string): string { } /** - * A wall-clock budget for one whole operation, not per spawned process: - * `concat` runs one encode per clip, so a per-command timeout would still - * multiply out to an unbounded total. Every spawn draws from the same deadline - * and is SIGKILLed when it expires or when the caller aborts. + * One deadline for the whole operation, not per spawned process: `concat` runs + * an encode per clip, so a per-command timeout would still multiply out to an + * unbounded total. Every spawn shares this signal and is SIGKILLed when it + * fires — whether from the deadline or the caller's own cancellation. */ -class OperationBudget { - private readonly deadline: number - - constructor( - timeoutMs: number, - readonly signal?: AbortSignal - ) { - this.deadline = Date.now() + timeoutMs - } - - remainingMs(): number { - return this.deadline - Date.now() - } - - assertLive(): void { - if (this.signal?.aborted) { - throw new Error('FFmpeg operation aborted') - } - if (this.remainingMs() <= 0) { - throw new Error(TIME_BUDGET_EXCEEDED) - } - } -} - interface RunContext { dir: string - budget: OperationBudget + limit: TimeoutAbortController } -function createBudget(runOptions: FfmpegRunOptions): OperationBudget { +function createOperationLimit(runOptions: FfmpegRunOptions): TimeoutAbortController { + const requested = runOptions.timeoutMs const timeoutMs = - Number.isFinite(runOptions.timeoutMs) && (runOptions.timeoutMs as number) > 0 - ? Math.min(runOptions.timeoutMs as number, DEFAULT_FFMPEG_TIMEOUT_MS) + typeof requested === 'number' && requested > 0 + ? Math.min(requested, DEFAULT_FFMPEG_TIMEOUT_MS) : DEFAULT_FFMPEG_TIMEOUT_MS - return new OperationBudget(timeoutMs, runOptions.signal) + return createTimeoutAbortController(timeoutMs, runOptions.signal) } -async function withTempDir( - budget: OperationBudget, - fn: (ctx: RunContext) => Promise -): Promise { - ensureFfmpeg() +/** Distinguishes "we ran out of time" from "the caller cancelled" for the message. */ +function abortError(limit: TimeoutAbortController): Error { + return new Error(limit.isTimedOut() ? TIME_BUDGET_EXCEEDED : 'FFmpeg operation aborted') +} + +function assertOperationLive(limit: TimeoutAbortController): void { + if (limit.signal.aborted) throw abortError(limit) +} + +async function withTempDir(fn: (dir: string) => Promise): Promise { const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'media-ffmpeg-')) try { - return await fn({ dir, budget }) + return await fn(dir) } finally { await fs.rm(dir, { recursive: true, force: true }).catch(() => {}) } } +/** + * Resolves a name inside the operation's temp dir and refuses anything that + * escapes it. The invariant this module needs is "nothing is read or written + * outside the temp dir" — asserting it here makes that structural, rather than + * depending on every present and future filename source remembering to + * sanitize itself. + */ +function tempPath(dir: string, name: string): string { + const resolved = path.resolve(dir, name) + if (resolved !== dir && !resolved.startsWith(dir + path.sep)) { + throw new Error(`Refusing to use a path outside the working directory: ${name}`) + } + return resolved +} + async function writeInput(dir: string, file: MediaFile, index: number): Promise { const ext = extFromMime(file.mimeType) - const filePath = path.join(dir, `in-${index}.${ext}`) + const filePath = tempPath(dir, `in-${index}.${ext}`) await fs.writeFile(filePath, file.buffer) return filePath } @@ -386,28 +386,27 @@ async function writeInput(dir: string, file: MediaFile, index: number): Promise< function runCommand( command: ffmpeg.FfmpegCommand, outputPath: string, - budget: OperationBudget + limit: TimeoutAbortController ): Promise { - budget.assertLive() + ensureFfmpeg() + assertOperationLive(limit) return new Promise((resolve, reject) => { let settled = false - function settle(err?: Error): void { - if (settled) return - settled = true - clearTimeout(timer) - budget.signal?.removeEventListener('abort', onAbort) - if (err) reject(err) - else resolve() - } /** SIGKILL, not SIGTERM: a wedged encoder must not get to ignore the signal. */ - function kill(reason: string): void { + function hardKill(): void { try { command.kill('SIGKILL') } catch { - // The process may already be gone; the rejection below is what matters. + // Already gone, or not yet spawned; onStart covers the latter. } - settle(new Error(reason)) + } + function settle(err?: Error): void { + if (settled) return + settled = true + limit.signal.removeEventListener('abort', onAbort) + if (err) reject(err) + else resolve() } /** * fluent-ffmpeg's kill() is a silent no-op until the child exists, and @@ -417,37 +416,25 @@ function runCommand( * it once the process is up. 'start' fires immediately after the spawn. */ function onStart(): void { - if (settled) { - try { - command.kill('SIGKILL') - } catch { - // Nothing to signal; the promise has already settled. - } - } + if (settled) hardKill() } function onAbort(): void { - kill('FFmpeg operation aborted') + hardKill() + settle(abortError(limit)) } - const timer = setTimeout(() => kill(TIME_BUDGET_EXCEEDED), budget.remainingMs()) - budget.signal?.addEventListener('abort', onAbort, { once: true }) - - command - .on('start', onStart) - .on('end', () => settle()) - .on('error', (err) => settle(new Error(`FFmpeg error: ${err.message}`))) - .save(outputPath) - }) -} - -export async function probeMedia( - file: MediaFile, - runOptions: FfmpegRunOptions = {} -): Promise { - const budget = createBudget(runOptions) - return withTempDir(budget, async ({ dir }) => { - const inputPath = await writeInput(dir, file, 0) - return probeFile(inputPath, budget) + limit.signal.addEventListener('abort', onAbort, { once: true }) + + try { + command + .on('start', onStart) + .on('end', () => settle()) + .on('error', (err) => settle(new Error(`FFmpeg error: ${err.message}`))) + .save(outputPath) + } catch (err) { + // Keeps the abort listener from outliving a command that never started. + settle(toError(err)) + } }) } @@ -485,18 +472,18 @@ function describeProbeFailure(err: Error & { killed?: boolean; code?: unknown }, * handle on the child and so cannot be killed: a crafted input that wedges * ffprobe would otherwise hang forever holding a request. */ -function probeFile(filePath: string, budget: OperationBudget): Promise { - budget.assertLive() - const timeout = Math.min(PROBE_TIMEOUT_MS, budget.remainingMs()) +function probeFile(filePath: string, limit: TimeoutAbortController): Promise { + assertOperationLive(limit) + const remaining = getRemainingExecutionMs(limit.signal) ?? PROBE_TIMEOUT_MS return new Promise((resolve, reject) => { execFile( resolveFfprobePath(), ['-v', 'error', '-print_format', 'json', '-show_format', '-show_streams', '-i', filePath], { - timeout, + timeout: Math.min(PROBE_TIMEOUT_MS, remaining), killSignal: 'SIGKILL', maxBuffer: PROBE_MAX_OUTPUT_BYTES, - signal: budget.signal, + signal: limit.signal, }, (err, stdout, stderr) => { if (err) { @@ -547,50 +534,49 @@ export async function runFfmpegOperation( ) } - assertOptionsWithinBounds(operation, options) + const limit = createOperationLimit(runOptions) + assertOperationLive(limit) - const budget = createBudget(runOptions) - budget.assertLive() + try { + return await withTempDir(async (dir) => { + const ctx: RunContext = { dir, limit } + if (operation === 'probe') { + return { probe: await probeFile(await writeInput(dir, inputs[0], 0), limit) } + } - if (operation === 'probe') { - return { - probe: await withTempDir(budget, async ({ dir }) => - probeFile(await writeInput(dir, inputs[0], 0), budget) - ), - } + const inputPaths = await Promise.all(inputs.map((f, i) => writeInput(dir, f, i))) + + switch (operation) { + case 'overlay_audio': + case 'mux': + return overlayAudio(ctx, inputPaths, options) + case 'mix_audio': + return mixAudio(ctx, inputPaths, options) + case 'concat': + return concat(ctx, inputPaths) + case 'trim': + return trim(ctx, inputPaths[0], inputs[0], options) + case 'scale_pad': + return scalePad(ctx, inputPaths[0], options) + case 'overlay_image': + return overlayImage(ctx, inputPaths, options) + case 'add_text': + return addText(ctx, inputPaths[0], options) + case 'fade': + return fade(ctx, inputPaths[0], inputs[0], options) + case 'extract_audio': + return extractAudio(ctx, inputPaths[0], options) + case 'convert': + return convert(ctx, inputPaths[0], options) + case 'thumbnail': + return thumbnail(ctx, inputPaths[0], options) + default: + throw new Error(`Unsupported ffmpeg operation: ${operation}`) + } + }) + } finally { + limit.cleanup() } - - return withTempDir(budget, async (ctx) => { - const inputPaths = await Promise.all(inputs.map((f, i) => writeInput(ctx.dir, f, i))) - - switch (operation) { - case 'overlay_audio': - case 'mux': - return overlayAudio(ctx, inputPaths, options) - case 'mix_audio': - return mixAudio(ctx, inputPaths, options) - case 'concat': - return concat(ctx, inputPaths) - case 'trim': - return trim(ctx, inputPaths[0], inputs[0], options) - case 'scale_pad': - return scalePad(ctx, inputPaths[0], options) - case 'overlay_image': - return overlayImage(ctx, inputPaths, options) - case 'add_text': - return addText(ctx, inputPaths[0], options) - case 'fade': - return fade(ctx, inputPaths[0], inputs[0], options) - case 'extract_audio': - return extractAudio(ctx, inputPaths[0], options) - case 'convert': - return convert(ctx, inputPaths[0], options) - case 'thumbnail': - return thumbnail(ctx, inputPaths[0], options) - default: - throw new Error(`Unsupported ffmpeg operation: ${operation}`) - } - }) } async function readOut(outputPath: string, ext: string): Promise { @@ -599,12 +585,12 @@ async function readOut(outputPath: string, ext: string): Promise { } async function overlayAudio( - { dir, budget }: RunContext, + { dir, limit }: RunContext, inputPaths: string[], options: FfmpegOptions ): Promise { if (inputPaths.length < 2) throw new Error('overlay_audio requires [video, audio]') - const outputPath = path.join(dir, 'out.mp4') + const outputPath = tempPath(dir, 'out.mp4') const command = ffmpeg().input(inputPaths[0]) if (options.loopToVideo) { command.input(inputPaths[1]).inputOptions(['-stream_loop', '-1']) @@ -622,17 +608,17 @@ async function overlayAudio( 'aac', '-shortest', ]) - await runCommand(command, outputPath, budget) + await runCommand(command, outputPath, limit) return readOut(outputPath, 'mp4') } async function mixAudio( - { dir, budget }: RunContext, + { dir, limit }: RunContext, inputPaths: string[], options: FfmpegOptions ): Promise { if (inputPaths.length < 2) throw new Error('mix_audio requires [voice, music]') - const outputPath = path.join(dir, 'out.mp3') + const outputPath = tempPath(dir, 'out.mp3') const voiceVol = resolveVolume(options.volume ?? 1, 'volume') const musicVol = resolveVolume(options.musicVolume ?? 0.3, 'musicVolume') const command = ffmpeg() @@ -644,13 +630,13 @@ async function mixAudio( `[v][m]amix=inputs=2:duration=longest:dropout_transition=0[a]`, ]) .outputOptions(['-map', '[a]']) - await runCommand(command, outputPath, budget) + await runCommand(command, outputPath, limit) return readOut(outputPath, 'mp3') } -async function concat({ dir, budget }: RunContext, inputPaths: string[]): Promise { +async function concat({ dir, limit }: RunContext, inputPaths: string[]): Promise { if (inputPaths.length < 2) throw new Error('concat requires at least 2 clips') - const probes = await Promise.all(inputPaths.map((p) => probeFile(p, budget))) + const probes = await Promise.all(inputPaths.map((p) => probeFile(p, limit))) probes.forEach((p, i) => { if (!p.hasVideo) { throw new Error( @@ -671,7 +657,7 @@ async function concat({ dir, budget }: RunContext, inputPaths: string[]): Promis // non-existent [i:a]), which is the "Error binding filtergraph inputs/outputs" failure. const normalized: string[] = [] for (let i = 0; i < inputPaths.length; i++) { - const out = path.join(dir, `norm-${i}.mp4`) + const out = tempPath(dir, `norm-${i}.mp4`) const cmd = ffmpeg().input(inputPaths[i]) const maps: string[] = ['-map', '0:v:0'] const extra: string[] = [] @@ -712,88 +698,99 @@ async function concat({ dir, budget }: RunContext, inputPaths: string[]): Promis '2', ...extra, ]) - await runCommand(cmd, out, budget) + await runCommand(cmd, out, limit) normalized.push(out) } // Concatenate the now-uniform clips with the concat demuxer (stream copy: fast + reliable). - const listPath = path.join(dir, 'concat-list.txt') + const listPath = tempPath(dir, 'concat-list.txt') await fs.writeFile( listPath, normalized.map((p) => `file '${p.replace(/'/g, "'\\''")}'`).join('\n') ) - const outputPath = path.join(dir, 'out.mp4') + const outputPath = tempPath(dir, 'out.mp4') const concatCmd = ffmpeg() .input(listPath) .inputOptions(['-f', 'concat', '-safe', '0']) .outputOptions(['-c', 'copy', '-movflags', '+faststart']) - await runCommand(concatCmd, outputPath, budget) + await runCommand(concatCmd, outputPath, limit) return readOut(outputPath, 'mp4') } async function trim( - { dir, budget }: RunContext, + { dir, limit }: RunContext, inputPath: string, input: MediaFile, options: FfmpegOptions ): Promise { const ext = extFromMime(input.mimeType) - const outputPath = path.join(dir, `out.${ext}`) + const outputPath = tempPath(dir, `out.${ext}`) const start = resolveNonNegativeSeconds(options.start ?? 0, 'start') const command = ffmpeg(inputPath).setStartTime(start) if (options.end !== undefined) { const end = resolveNonNegativeSeconds(options.end, 'end') - command.setDuration(Math.max(0, end - start)) + // Without this, `end < start` clamps to a zero-length output that is written + // to the workspace and reported as a success. + if (end < start) { + throw new Error(`end (${end}s) must be greater than or equal to start (${start}s)`) + } + command.setDuration(end - start) } - await runCommand(command, outputPath, budget) + await runCommand(command, outputPath, limit) return readOut(outputPath, ext) } async function scalePad( - { dir, budget }: RunContext, + { dir, limit }: RunContext, inputPath: string, options: FfmpegOptions ): Promise { let width = options.width let height = options.height - if ((!width || !height) && options.aspectRatio && ASPECT_TARGETS[options.aspectRatio]) { + // Only an omitted dimension falls back to the aspect ratio. A supplied 0 is a + // bad value, not an absent one, and must reach the bounds check to say so. + if ( + (width === undefined || height === undefined) && + options.aspectRatio && + ASPECT_TARGETS[options.aspectRatio] + ) { width = ASPECT_TARGETS[options.aspectRatio].w height = ASPECT_TARGETS[options.aspectRatio].h } - if (!width || !height) { + if (width === undefined || height === undefined) { throw new Error('scale_pad requires width+height or a known aspectRatio (e.g. 9:16)') } const scaleWidth = resolveScaleDimension(width, 'width') const scaleHeight = resolveScaleDimension(height, 'height') - const outputPath = path.join(dir, 'out.mp4') + const outputPath = tempPath(dir, 'out.mp4') const command = ffmpeg(inputPath) .videoFilters( `scale=${scaleWidth}:${scaleHeight}:force_original_aspect_ratio=decrease,pad=${scaleWidth}:${scaleHeight}:(ow-iw)/2:(oh-ih)/2,setsar=1` ) .outputOptions(['-c:a', 'copy']) - await runCommand(command, outputPath, budget) + await runCommand(command, outputPath, limit) return readOut(outputPath, 'mp4') } async function overlayImage( - { dir, budget }: RunContext, + { dir, limit }: RunContext, inputPaths: string[], options: FfmpegOptions ): Promise { if (inputPaths.length < 2) throw new Error('overlay_image requires [video, image]') const xy = OVERLAY_POSITION[options.position || 'top-right'] || OVERLAY_POSITION['top-right'] - const outputPath = path.join(dir, 'out.mp4') + const outputPath = tempPath(dir, 'out.mp4') const command = ffmpeg() .input(inputPaths[0]) .input(inputPaths[1]) .complexFilter([`[0:v][1:v]overlay=${xy}[v]`]) .outputOptions(['-map', '[v]', '-map', '0:a?', '-c:a', 'copy']) - await runCommand(command, outputPath, budget) + await runCommand(command, outputPath, limit) return readOut(outputPath, 'mp4') } async function addText( - { dir, budget }: RunContext, + { dir, limit }: RunContext, inputPath: string, options: FfmpegOptions ): Promise { @@ -809,70 +806,70 @@ async function addText( `x=${pos.x}`, `y=${pos.y}`, ].join(':') - const outputPath = path.join(dir, 'out.mp4') + const outputPath = tempPath(dir, 'out.mp4') const command = ffmpeg(inputPath) .videoFilters(`drawtext=${drawtext}`) .outputOptions(['-c:a', 'copy']) - await runCommand(command, outputPath, budget) + await runCommand(command, outputPath, limit) return readOut(outputPath, 'mp4') } async function fade( - { dir, budget }: RunContext, + { dir, limit }: RunContext, inputPath: string, input: MediaFile, _options: FfmpegOptions ): Promise { - const probe = await probeFile(inputPath, budget) + const probe = await probeFile(inputPath, limit) const duration = probe.durationSeconds || 0 const fadeDur = Math.min(0.5, duration / 4 || 0.5) const outStart = Math.max(0, duration - fadeDur) const isVideo = input.mimeType.startsWith('video/') || probe.hasVideo const ext = isVideo ? 'mp4' : extFromMime(input.mimeType) - const outputPath = path.join(dir, `out.${ext}`) + const outputPath = tempPath(dir, `out.${ext}`) const command = ffmpeg(inputPath) if (isVideo) { command.videoFilters([`fade=t=in:st=0:d=${fadeDur}`, `fade=t=out:st=${outStart}:d=${fadeDur}`]) } command.audioFilters([`afade=t=in:st=0:d=${fadeDur}`, `afade=t=out:st=${outStart}:d=${fadeDur}`]) - await runCommand(command, outputPath, budget) + await runCommand(command, outputPath, limit) return readOut(outputPath, ext) } async function extractAudio( - { dir, budget }: RunContext, + { dir, limit }: RunContext, inputPath: string, options: FfmpegOptions ): Promise { - const ext = resolveOutputExt(options.format || 'mp3') - const outputPath = path.join(dir, `out.${ext}`) + const ext = resolveOutputExt(options.format || 'mp3', AUDIO_EXTS) + const outputPath = tempPath(dir, `out.${ext}`) const command = ffmpeg(inputPath).noVideo() - await runCommand(command, outputPath, budget) + await runCommand(command, outputPath, limit) return readOut(outputPath, ext) } async function convert( - { dir, budget }: RunContext, + { dir, limit }: RunContext, inputPath: string, options: FfmpegOptions ): Promise { if (!options.format) throw new Error('convert requires a target format') const ext = resolveOutputExt(options.format) - const outputPath = path.join(dir, `out.${ext}`) - await runCommand(ffmpeg(inputPath), outputPath, budget) + const outputPath = tempPath(dir, `out.${ext}`) + await runCommand(ffmpeg(inputPath), outputPath, limit) return readOut(outputPath, ext) } async function thumbnail( - { dir, budget }: RunContext, + { dir, limit }: RunContext, inputPath: string, options: FfmpegOptions ): Promise { - const outputPath = path.join(dir, 'out.jpg') + const outputPath = tempPath(dir, 'out.jpg') const command = ffmpeg(inputPath) .seekInput(resolveNonNegativeSeconds(options.start ?? 0, 'start')) .frames(1) - await runCommand(command, outputPath, budget) + await runCommand(command, outputPath, limit) return readOut(outputPath, 'jpg') } From acc3c6dd8c5cc2a8ae6546321c9931eed538c3f0 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 10:39:17 -0700 Subject: [PATCH 4/8] chore(media): drop comments that restate the code they annotate Two narrated the adjacent literal or ternary. The third was orphaned by the deleted validation chain, so TSDoc bound it to ASPECT_TARGETS and documented the wrong declaration; its rationale is in the commit that removed the chain. --- apps/sim/lib/media/ffmpeg.ts | 8 -------- 1 file changed, 8 deletions(-) diff --git a/apps/sim/lib/media/ffmpeg.ts b/apps/sim/lib/media/ffmpeg.ts index be45134582b..335f3a4985c 100644 --- a/apps/sim/lib/media/ffmpeg.ts +++ b/apps/sim/lib/media/ffmpeg.ts @@ -74,7 +74,6 @@ export const DEFAULT_FFMPEG_TIMEOUT_MS = 5 * 60 * 1000 const PROBE_TIMEOUT_MS = 15 * 1000 const PROBE_MAX_OUTPUT_BYTES = 8 * 1024 * 1024 -/** Names the actionable cause: the budget covers all clips, so fewer/shorter inputs is the fix. */ const TIME_BUDGET_EXCEEDED = 'FFmpeg operation exceeded its time budget — try fewer, shorter, or lower-resolution inputs' @@ -283,12 +282,6 @@ function resolveVolume(value: number, label: string): number { return value } -/** - * Each operation validates the options it consumes, at the point of use, so a - * rule has exactly one home. An LLM caller routinely emits surplus parameters, - * so an operation must ignore — never reject over — a value it never reads. - */ - const ASPECT_TARGETS: Record = { '16:9': { w: 1920, h: 1080 }, '9:16': { w: 1080, h: 1920 }, @@ -343,7 +336,6 @@ function createOperationLimit(runOptions: FfmpegRunOptions): TimeoutAbortControl return createTimeoutAbortController(timeoutMs, runOptions.signal) } -/** Distinguishes "we ran out of time" from "the caller cancelled" for the message. */ function abortError(limit: TimeoutAbortController): Error { return new Error(limit.isTimedOut() ? TIME_BUDGET_EXCEEDED : 'FFmpeg operation aborted') } From 79d78e540d7f910e5f23e884896950a353a9aaec Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 10:48:30 -0700 Subject: [PATCH 5/8] fix(media): let a host with only ffprobe still probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ensureFfmpeg() conflated 'resolve the binary' with 'require the binary', and resolveFfprobePath called it first — so on a host with FFPROBE_PATH set but no ffmpeg, the first probe succeeded and every later one threw, because the failed lookup is memoized. Split the non-throwing init from the ffmpeg-required assertion; transcoding still demands ffmpeg, probing no longer does. Covered by a test in its own file, since the binary lookup memoizes at module scope and a shared file would already have consumed that state. --- .../lib/media/ffmpeg-probe-resolution.test.ts | 62 +++++++++++++++++++ apps/sim/lib/media/ffmpeg.ts | 43 +++++++------ 2 files changed, 87 insertions(+), 18 deletions(-) create mode 100644 apps/sim/lib/media/ffmpeg-probe-resolution.test.ts diff --git a/apps/sim/lib/media/ffmpeg-probe-resolution.test.ts b/apps/sim/lib/media/ffmpeg-probe-resolution.test.ts new file mode 100644 index 00000000000..d1df04ec3de --- /dev/null +++ b/apps/sim/lib/media/ffmpeg-probe-resolution.test.ts @@ -0,0 +1,62 @@ +/** + * Standalone file: the ffmpeg module memoizes its binary lookup at module + * scope, so exercising the "no ffmpeg installed" branch needs a fresh module + * state that a shared file would already have consumed. + * + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { execSyncMock, execFileMock } = vi.hoisted(() => ({ + execSyncMock: vi.fn(), + execFileMock: vi.fn(), +})) + +vi.mock('node:child_process', () => ({ + execSync: execSyncMock, + execFile: execFileMock, +})) + +import { runFfmpegOperation } from '@/lib/media/ffmpeg' + +const PROBE_JSON = JSON.stringify({ + format: { duration: '3', format_name: 'mov,mp4' }, + streams: [{ codec_type: 'video', codec_name: 'h264', width: 640, height: 480 }], +}) + +describe('probing without a discoverable ffmpeg binary', () => { + beforeEach(() => { + vi.clearAllMocks() + // No ffmpeg on this host. + execSyncMock.mockImplementation(() => { + throw new Error('which: no ffmpeg in PATH') + }) + execFileMock.mockImplementation((_bin, _args, _opts, cb) => { + cb(null, PROBE_JSON, '') + return {} + }) + }) + + it('keeps probing across repeated calls', async () => { + const file = { buffer: Buffer.from('media'), mimeType: 'video/mp4' } + + // The second call is the regression: the binary lookup is memoized after + // the first, and an ffmpeg-required check here would throw from then on + // even though ffprobe is perfectly usable. + for (const _ of [1, 2, 3]) { + const result = await runFfmpegOperation('probe', [file]) + expect(result.probe).toMatchObject({ hasVideo: true, width: 640, height: 480 }) + } + + expect(execFileMock).toHaveBeenCalledTimes(3) + expect(execFileMock.mock.calls[0][0]).toContain('ffprobe') + }) + + it('still refuses to transcode, which genuinely needs ffmpeg', async () => { + await expect( + runFfmpegOperation('convert', [{ buffer: Buffer.from('m'), mimeType: 'video/mp4' }], { + format: 'mp3', + }) + ).rejects.toThrow('FFmpeg not found') + }) +}) diff --git a/apps/sim/lib/media/ffmpeg.ts b/apps/sim/lib/media/ffmpeg.ts index 335f3a4985c..619a1fc6001 100644 --- a/apps/sim/lib/media/ffmpeg.ts +++ b/apps/sim/lib/media/ffmpeg.ts @@ -18,16 +18,9 @@ let ffmpegInitialized = false let ffmpegPath: string | null = null let ffprobePath: string | null = null -/** Lazy system FFmpeg binary resolution, mirroring lib/audio/extractor.ts. */ -function ensureFfmpeg(): void { - if (ffmpegInitialized) { - if (!ffmpegPath) { - throw new Error( - 'FFmpeg not found. Install: brew install ffmpeg (macOS) / apk add ffmpeg (Alpine) / apt-get install ffmpeg (Ubuntu)' - ) - } - return - } +/** Lazy system FFmpeg binary resolution, mirroring lib/audio/extractor.ts. Never throws. */ +function initFfmpegPath(): void { + if (ffmpegInitialized) return ffmpegInitialized = true try { @@ -39,25 +32,39 @@ function ensureFfmpeg(): void { } } +/** + * Transcoding requires ffmpeg itself. Probing does not — kept separate from + * {@link initFfmpegPath} so a host with only ffprobe can still probe. + */ +function ensureFfmpeg(): void { + initFfmpegPath() + if (!ffmpegPath) { + throw new Error( + 'FFmpeg not found. Install: brew install ffmpeg (macOS) / apk add ffmpeg (Alpine) / apt-get install ffmpeg (Ubuntu)' + ) + } +} + /** * Mirrors fluent-ffmpeg's resolution order (FFPROBE_PATH, then PATH, then * ffmpeg's own directory) so replacing its ffprobe call does not narrow where * the binary may live for self-hosters. */ function resolveFfprobePath(): string { - ensureFfmpeg() if (ffprobePath) return ffprobePath const binary = process.platform === 'win32' ? 'ffprobe.exe' : 'ffprobe' const configured = process.env.FFPROBE_PATH?.trim() - const sibling = ffmpegPath ? path.join(path.dirname(ffmpegPath), binary) : undefined + if (configured && existsSync(configured)) { + ffprobePath = configured + return ffprobePath + } - ffprobePath = - configured && existsSync(configured) - ? configured - : sibling && existsSync(sibling) - ? sibling - : binary + // Deliberately initFfmpegPath, not ensureFfmpeg: a missing ffmpeg must not + // stop a probe, since ffprobe may still be on PATH. + initFfmpegPath() + const sibling = ffmpegPath ? path.join(path.dirname(ffmpegPath), binary) : undefined + ffprobePath = sibling && existsSync(sibling) ? sibling : binary return ffprobePath } From 4560e5a99f685a2fd1fcea779140d630f127bee3 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 10:54:20 -0700 Subject: [PATCH 6/8] fix(media): drop weba from the output allowlist FFmpeg's muxer is named webm and refuses a .weba output ('Error initializing the muxer'), so allowlisting the extension only converted a clear 'unsupported format' rejection into a confusing encode-time failure. weba was added in this PR because it appears in the input MIME map, but naming an input file and naming an output muxer are different questions. extract_audio takes webm. --- apps/sim/lib/media/ffmpeg.test.ts | 10 +++++++++- apps/sim/lib/media/ffmpeg.ts | 11 +++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/apps/sim/lib/media/ffmpeg.test.ts b/apps/sim/lib/media/ffmpeg.test.ts index ef96c192074..ef76e5f5d13 100644 --- a/apps/sim/lib/media/ffmpeg.test.ts +++ b/apps/sim/lib/media/ffmpeg.test.ts @@ -57,10 +57,18 @@ describe('runFfmpegOperation output format validation', () => { (e: Error) => e ) - for (const format of ['mp4', 'mov', 'webm', 'mp3', 'wav', 'gif', 'webp', 'weba']) { + for (const format of ['mp4', 'mov', 'webm', 'mp3', 'wav', 'gif', 'webp']) { expect(error.message).toContain(format) } }) + + it('rejects weba, which FFmpeg has no muxer for', async () => { + // The extension appears in the input MIME map, but `ffmpeg out.weba` fails + // with "Error initializing the muxer" — webm is the muxer's real name. + await expect(runFfmpegOperation('convert', [mediaFile()], { format: 'weba' })).rejects.toThrow( + 'Unsupported output format' + ) + }) }) describe('runFfmpegOperation scale bounds', () => { diff --git a/apps/sim/lib/media/ffmpeg.ts b/apps/sim/lib/media/ffmpeg.ts index 619a1fc6001..12ac5ddea44 100644 --- a/apps/sim/lib/media/ffmpeg.ts +++ b/apps/sim/lib/media/ffmpeg.ts @@ -189,7 +189,6 @@ const EXT_TO_MIME: Record = { flac: 'audio/flac', aac: 'audio/aac', opus: 'audio/opus', - weba: 'audio/webm', png: 'image/png', jpg: 'image/jpeg', jpeg: 'image/jpeg', @@ -216,7 +215,6 @@ const OUTPUT_EXTS = new Set([ 'flac', 'aac', 'opus', - 'weba', 'png', 'jpg', 'jpeg', @@ -224,8 +222,13 @@ const OUTPUT_EXTS = new Set([ 'webp', ]) -/** extract_audio can only name an audio container; the rest would silently produce nothing useful. */ -const AUDIO_EXTS = new Set(['mp3', 'm4a', 'wav', 'ogg', 'flac', 'aac', 'opus', 'weba']) +/** + * extract_audio can only name an audio container; the rest would silently + * produce nothing useful. `webm`, not `weba` — FFmpeg's muxer is named webm and + * it refuses a .weba output, so allowlisting that extension would only produce + * a muxer error at encode time. + */ +const AUDIO_EXTS = new Set(['mp3', 'm4a', 'wav', 'ogg', 'flac', 'aac', 'opus', 'webm']) /** * Temp-file names are built as `${prefix}.${ext}` and joined against the temp From 6c7a25e87ba97f30b84d8f2ae31130c4d543916c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 11:01:00 -0700 Subject: [PATCH 7/8] fix(media): look ffprobe up on PATH before ffmpeg's sibling The TSDoc claimed fluent-ffmpeg's order (FFPROBE_PATH, then PATH, then ffmpeg's directory) while the code checked the sibling second, so a stray or unusable file next to the ffmpeg binary would be cached and mask a working PATH install for every probe. Match the documented order. Pinned by a test in its own file: the resolved path memoizes at module scope, so precedence is only observable in a module no other test has resolved in. It mocks existsSync as well as execSync, without which the sibling never exists on the test host and the two orderings are indistinguishable. --- .../lib/media/ffmpeg-probe-precedence.test.ts | 50 +++++++++++++++++++ apps/sim/lib/media/ffmpeg.ts | 24 +++++++-- 2 files changed, 70 insertions(+), 4 deletions(-) create mode 100644 apps/sim/lib/media/ffmpeg-probe-precedence.test.ts diff --git a/apps/sim/lib/media/ffmpeg-probe-precedence.test.ts b/apps/sim/lib/media/ffmpeg-probe-precedence.test.ts new file mode 100644 index 00000000000..673e21f53b3 --- /dev/null +++ b/apps/sim/lib/media/ffmpeg-probe-precedence.test.ts @@ -0,0 +1,50 @@ +/** + * Standalone file: the resolved ffprobe path is memoized at module scope, so + * the first resolution in a process wins. Testing precedence therefore needs a + * module whose memo no other test has populated. + * + * @vitest-environment node + */ +import { describe, expect, it, vi } from 'vitest' + +const { execSyncMock, execFileMock, existsSyncMock } = vi.hoisted(() => ({ + execSyncMock: vi.fn(), + execFileMock: vi.fn(), + existsSyncMock: vi.fn(), +})) + +vi.mock('node:child_process', () => ({ + execSync: execSyncMock, + execFile: execFileMock, +})) + +vi.mock('node:fs', () => ({ + existsSync: existsSyncMock, +})) + +import { runFfmpegOperation } from '@/lib/media/ffmpeg' + +describe('ffprobe lookup precedence', () => { + it('prefers ffprobe on PATH over a sibling of the ffmpeg binary', async () => { + // ffmpeg resolves into a directory whose ffprobe sibling may be stray or + // unusable; a real PATH entry must win. Both lookups go through execSync, + // so they are distinguished by the command. + execSyncMock.mockImplementation((cmd: string) => { + if (cmd.includes('ffprobe')) return '/usr/bin/ffprobe\n' + if (cmd.includes('ffmpeg')) return '/opt/broken/ffmpeg\n' + throw new Error(`unexpected command: ${cmd}`) + }) + // The sibling exists on disk — without this the test cannot tell the two + // orderings apart, because a non-existent sibling is skipped either way. + existsSyncMock.mockImplementation((p: string) => p === '/opt/broken/ffprobe') + execFileMock.mockImplementation((_bin, _args, _opts, cb) => { + cb(null, JSON.stringify({ format: {}, streams: [] }), '') + return {} + }) + + await runFfmpegOperation('probe', [{ buffer: Buffer.from('media'), mimeType: 'video/mp4' }]) + + expect(execFileMock.mock.calls[0][0]).toBe('/usr/bin/ffprobe') + expect(execFileMock.mock.calls[0][0]).not.toBe('/opt/broken/ffprobe') + }) +}) diff --git a/apps/sim/lib/media/ffmpeg.ts b/apps/sim/lib/media/ffmpeg.ts index 12ac5ddea44..d37f7bd4de4 100644 --- a/apps/sim/lib/media/ffmpeg.ts +++ b/apps/sim/lib/media/ffmpeg.ts @@ -45,10 +45,20 @@ function ensureFfmpeg(): void { } } +function lookupOnPath(binary: string): string | null { + try { + const cmd = process.platform === 'win32' ? `where ${binary}` : `which ${binary}` + return execSync(cmd, { encoding: 'utf-8' }).trim().split('\n')[0] || null + } catch { + return null + } +} + /** - * Mirrors fluent-ffmpeg's resolution order (FFPROBE_PATH, then PATH, then - * ffmpeg's own directory) so replacing its ffprobe call does not narrow where - * the binary may live for self-hosters. + * Mirrors fluent-ffmpeg's resolution order — FFPROBE_PATH, then PATH, then + * ffmpeg's own directory — so replacing its ffprobe call does not narrow where + * the binary may live for self-hosters. PATH outranks the sibling deliberately: + * a stray or unusable file next to ffmpeg must not mask a working install. */ function resolveFfprobePath(): string { if (ffprobePath) return ffprobePath @@ -60,8 +70,14 @@ function resolveFfprobePath(): string { return ffprobePath } + const onPath = lookupOnPath(binary) + if (onPath) { + ffprobePath = onPath + return ffprobePath + } + // Deliberately initFfmpegPath, not ensureFfmpeg: a missing ffmpeg must not - // stop a probe, since ffprobe may still be on PATH. + // stop a probe when ffprobe is installed on its own. initFfmpegPath() const sibling = ffmpegPath ? path.join(path.dirname(ffmpegPath), binary) : undefined ffprobePath = sibling && existsSync(sibling) ? sibling : binary From dc6cc8bf6cd094d1f83eb67338b476bf9e54fb95 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 11:06:24 -0700 Subject: [PATCH 8/8] fix(media): floor the probe timeout and label audio-only webm correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from review: - Node reads execFile's `timeout: 0` as 'no timeout', so once the shared budget was spent the 15s probe cap disappeared entirely — the opposite of what an exhausted budget should do. Reachable between the deadline passing and the abort timer firing, where assertOperationLive still sees a live signal. Floor the computed cap at 1ms. - extract_audio accepts webm, but mimeFromExt resolves that container to video/webm, so an audio-only extract was stored with a video content type. Resolve audio-only outputs through a small override map. --- .../lib/media/ffmpeg-probe-resolution.test.ts | 11 +++++++++ apps/sim/lib/media/ffmpeg.test.ts | 9 +++++++ apps/sim/lib/media/ffmpeg.ts | 24 +++++++++++++++---- 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/apps/sim/lib/media/ffmpeg-probe-resolution.test.ts b/apps/sim/lib/media/ffmpeg-probe-resolution.test.ts index d1df04ec3de..05d2afbae58 100644 --- a/apps/sim/lib/media/ffmpeg-probe-resolution.test.ts +++ b/apps/sim/lib/media/ffmpeg-probe-resolution.test.ts @@ -52,6 +52,17 @@ describe('probing without a discoverable ffmpeg binary', () => { expect(execFileMock.mock.calls[0][0]).toContain('ffprobe') }) + it('always hands ffprobe a positive timeout', async () => { + // Node reads `timeout: 0` as "no timeout", so the computed cap is floored. + // Asserted on a healthy budget rather than an expired one: forcing the + // expired window means racing the abort timer, which makes the test flaky. + await runFfmpegOperation('probe', [{ buffer: Buffer.from('media'), mimeType: 'video/mp4' }]) + + const opts = execFileMock.mock.calls[0][2] as { timeout: number } + expect(opts.timeout).toBeGreaterThan(0) + expect(opts.timeout).toBeLessThanOrEqual(15_000) + }) + it('still refuses to transcode, which genuinely needs ffmpeg', async () => { await expect( runFfmpegOperation('convert', [{ buffer: Buffer.from('m'), mimeType: 'video/mp4' }], { diff --git a/apps/sim/lib/media/ffmpeg.test.ts b/apps/sim/lib/media/ffmpeg.test.ts index ef76e5f5d13..99ce87b8b3d 100644 --- a/apps/sim/lib/media/ffmpeg.test.ts +++ b/apps/sim/lib/media/ffmpeg.test.ts @@ -104,6 +104,15 @@ describe('runFfmpegOperation per-operation validation', () => { ) }) + it('allows webm for extract_audio but not weba', async () => { + const error = await runFfmpegOperation('extract_audio', [mediaFile()], { + format: 'weba', + }).catch((e: Error) => e) + + expect(error.message).toContain('Unsupported output format') + expect(error.message).toContain('webm') + }) + it('restricts extract_audio to audio containers', async () => { await expect( runFfmpegOperation('extract_audio', [mediaFile()], { format: 'png' }) diff --git a/apps/sim/lib/media/ffmpeg.ts b/apps/sim/lib/media/ffmpeg.ts index d37f7bd4de4..2f850106c39 100644 --- a/apps/sim/lib/media/ffmpeg.ts +++ b/apps/sim/lib/media/ffmpeg.ts @@ -246,6 +246,11 @@ const OUTPUT_EXTS = new Set([ */ const AUDIO_EXTS = new Set(['mp3', 'm4a', 'wav', 'ogg', 'flac', 'aac', 'opus', 'webm']) +/** Containers shared with video, whose content type differs for an audio-only output. */ +const AUDIO_ONLY_MIME: Record = { + webm: 'audio/webm', +} + /** * Temp-file names are built as `${prefix}.${ext}` and joined against the temp * dir, so an extension carrying `/` or `..` escapes that dir once `path.join` @@ -493,12 +498,17 @@ function describeProbeFailure(err: Error & { killed?: boolean; code?: unknown }, function probeFile(filePath: string, limit: TimeoutAbortController): Promise { assertOperationLive(limit) const remaining = getRemainingExecutionMs(limit.signal) ?? PROBE_TIMEOUT_MS + // Floored at 1ms: Node reads `timeout: 0` as "no timeout", so an expired + // budget would otherwise remove the probe cap entirely — the opposite of what + // an exhausted budget should do. Reachable between the deadline passing and + // the abort timer firing, where assertOperationLive still sees a live signal. + const timeout = Math.max(1, Math.min(PROBE_TIMEOUT_MS, remaining)) return new Promise((resolve, reject) => { execFile( resolveFfprobePath(), ['-v', 'error', '-print_format', 'json', '-show_format', '-show_streams', '-i', filePath], { - timeout: Math.min(PROBE_TIMEOUT_MS, remaining), + timeout, killSignal: 'SIGKILL', maxBuffer: PROBE_MAX_OUTPUT_BYTES, signal: limit.signal, @@ -597,9 +607,13 @@ export async function runFfmpegOperation( } } -async function readOut(outputPath: string, ext: string): Promise { +async function readOut( + outputPath: string, + ext: string, + contentType = mimeFromExt(ext) +): Promise { const buffer = await fs.readFile(outputPath) - return { buffer, ext, contentType: mimeFromExt(ext) } + return { buffer, ext, contentType } } async function overlayAudio( @@ -863,7 +877,9 @@ async function extractAudio( const outputPath = tempPath(dir, `out.${ext}`) const command = ffmpeg(inputPath).noVideo() await runCommand(command, outputPath, limit) - return readOut(outputPath, ext) + // A container shared with video (webm) resolves to a video content type by + // default, but this output has had its video stream dropped. + return readOut(outputPath, ext, AUDIO_ONLY_MIME[ext] ?? mimeFromExt(ext)) } async function convert(