Skip to content

Commit 4dc5a67

Browse files
committed
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.
1 parent fee45e2 commit 4dc5a67

4 files changed

Lines changed: 478 additions & 93 deletions

File tree

apps/sim/lib/copilot/tools/server/media/ffmpeg.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ vi.mock('@/lib/copilot/application/execute-file-use-case', () => ({
4242
}))
4343

4444
vi.mock('@/lib/media/ffmpeg', () => ({
45+
MAX_FFMPEG_INPUTS: 10,
4546
runFfmpegOperation: runFfmpegOperationMock,
4647
}))
4748

@@ -308,4 +309,37 @@ describe('ffmpeg server tool secret provenance', () => {
308309
message: 'ffmpeg convert failed: The media operation failed safely',
309310
})
310311
})
312+
313+
it('rejects more inputs than the cap before downloading any of them', async () => {
314+
const result = await ffmpegServerTool.execute(
315+
{
316+
operation: 'concat',
317+
inputs: {
318+
files: Array.from({ length: 11 }, () => ({ path: 'files/input.mp4' })),
319+
},
320+
},
321+
context
322+
)
323+
324+
expect(result.success).toBe(false)
325+
expect(result.message).toContain('At most 10 input files')
326+
expect(resolveWorkspaceFileReferenceMock).not.toHaveBeenCalled()
327+
expect(runFfmpegOperationMock).not.toHaveBeenCalled()
328+
})
329+
330+
it('forwards the abort signal so a cancelled turn can kill the transcode', async () => {
331+
const abortSignal = new AbortController().signal
332+
333+
await ffmpegServerTool.execute(
334+
{ operation: 'convert', inputs: { files: [{ path: 'files/input.mp4' }] } },
335+
{ ...context, abortSignal }
336+
)
337+
338+
expect(runFfmpegOperationMock).toHaveBeenCalledWith(
339+
'convert',
340+
expect.anything(),
341+
expect.anything(),
342+
{ signal: abortSignal }
343+
)
344+
})
311345
})

apps/sim/lib/copilot/tools/server/media/ffmpeg.ts

Lines changed: 45 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,12 @@ import {
1212
} from '@/lib/copilot/tools/server/base-tool'
1313
import { writeCopilotWorkspaceFileByPath } from '@/lib/copilot/vfs/resource-writer'
1414
import { MAX_MEDIA_BYTES } from '@/lib/media/falai'
15-
import { type FfmpegOperation, type MediaFile, runFfmpegOperation } from '@/lib/media/ffmpeg'
15+
import {
16+
type FfmpegOperation,
17+
MAX_FFMPEG_INPUTS,
18+
type MediaFile,
19+
runFfmpegOperation,
20+
} from '@/lib/media/ffmpeg'
1621
import {
1722
createWorkspaceFileSecretProvenanceFromRegistry,
1823
getBoundWorkspaceFileSecretProvenance,
@@ -71,6 +76,19 @@ interface FfmpegResult {
7176
probe?: unknown
7277
}
7378

79+
/**
80+
* A transcode outlives its request unless the child process is killed, so both
81+
* the transport abort and the explicit user stop must reach FFmpeg — checking
82+
* them only between steps leaves a cancelled turn burning cores.
83+
*/
84+
function resolveFfmpegAbortSignal(context: ServerToolContext): AbortSignal | undefined {
85+
const signals = [context.abortSignal, context.userStopSignal].filter(
86+
(signal): signal is AbortSignal => Boolean(signal)
87+
)
88+
if (signals.length === 0) return undefined
89+
return signals.length === 1 ? signals[0] : AbortSignal.any(signals)
90+
}
91+
7492
export const ffmpegServerTool: BaseServerTool<FfmpegArgs, FfmpegResult> = {
7593
name: Ffmpeg.id,
7694

@@ -90,6 +108,14 @@ export const ffmpegServerTool: BaseServerTool<FfmpegArgs, FfmpegResult> = {
90108
if (inputPaths.length === 0) {
91109
return { success: false, message: 'At least one input file is required in inputs.files' }
92110
}
111+
// Bounded before any download: the byte budget alone still permits hundreds
112+
// of small clips, and concat re-encodes every one of them serially.
113+
if (inputPaths.length > MAX_FFMPEG_INPUTS) {
114+
return {
115+
success: false,
116+
message: `At most ${MAX_FFMPEG_INPUTS} input files are allowed per ffmpeg operation (got ${inputPaths.length}).`,
117+
}
118+
}
93119

94120
let inputRequiresOpaqueError = false
95121
try {
@@ -138,19 +164,24 @@ export const ffmpegServerTool: BaseServerTool<FfmpegArgs, FfmpegResult> = {
138164
inputRequiresOpaqueError ||=
139165
inputProvenance.status === 'unknown' || inputProvenance.entries.length > 0
140166
assertServerToolNotAborted(context)
141-
const result = await runFfmpegOperation(params.operation, mediaFiles, {
142-
text: params.text,
143-
position: params.position,
144-
start: params.start,
145-
end: params.end,
146-
width: params.width,
147-
height: params.height,
148-
aspectRatio: params.aspectRatio,
149-
volume: params.volume,
150-
musicVolume: params.musicVolume,
151-
loopToVideo: params.loopToVideo,
152-
format: params.format,
153-
})
167+
const result = await runFfmpegOperation(
168+
params.operation,
169+
mediaFiles,
170+
{
171+
text: params.text,
172+
position: params.position,
173+
start: params.start,
174+
end: params.end,
175+
width: params.width,
176+
height: params.height,
177+
aspectRatio: params.aspectRatio,
178+
volume: params.volume,
179+
musicVolume: params.musicVolume,
180+
loopToVideo: params.loopToVideo,
181+
format: params.format,
182+
},
183+
{ signal: resolveFfmpegAbortSignal(context) }
184+
)
154185

155186
// probe reports metadata only — no file written.
156187
if (params.operation === 'probe') {

apps/sim/lib/media/ffmpeg.test.ts

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import { MAX_FFMPEG_INPUTS, type MediaFile, runFfmpegOperation } from '@/lib/media/ffmpeg'
6+
7+
function mediaFile(mimeType = 'video/mp4'): MediaFile {
8+
return { buffer: Buffer.from('media'), mimeType, name: 'clip.mp4' }
9+
}
10+
11+
describe('runFfmpegOperation input bounds', () => {
12+
it('rejects more inputs than the cap before touching the filesystem', async () => {
13+
const inputs = Array.from({ length: MAX_FFMPEG_INPUTS + 1 }, () => mediaFile())
14+
15+
await expect(runFfmpegOperation('concat', inputs)).rejects.toThrow(
16+
`At most ${MAX_FFMPEG_INPUTS} input files`
17+
)
18+
})
19+
20+
it('still requires at least one input', async () => {
21+
await expect(runFfmpegOperation('convert', [], { format: 'mp3' })).rejects.toThrow(
22+
'At least one input file is required'
23+
)
24+
})
25+
})
26+
27+
describe('runFfmpegOperation output format validation', () => {
28+
it.each([
29+
['../../escape.mp4', 'traversal'],
30+
['../pwned.mp3', 'parent segment'],
31+
['/etc/cron.d/x.mp4', 'absolute path'],
32+
['mp4/../../x', 'embedded separator'],
33+
])('rejects %s as an output format (%s)', async (format) => {
34+
await expect(runFfmpegOperation('convert', [mediaFile()], { format })).rejects.toThrow(
35+
'Unsupported output format'
36+
)
37+
})
38+
39+
it('rejects a format with no known muxer', async () => {
40+
await expect(runFfmpegOperation('convert', [mediaFile()], { format: 'exe' })).rejects.toThrow(
41+
'Unsupported output format'
42+
)
43+
})
44+
45+
it('rejects a traversal format on extract_audio too', async () => {
46+
await expect(
47+
runFfmpegOperation('extract_audio', [mediaFile()], { format: '../../escape.mp3' })
48+
).rejects.toThrow('Unsupported output format')
49+
})
50+
})
51+
52+
describe('runFfmpegOperation scale bounds', () => {
53+
it.each([
54+
[30000, 30000],
55+
[1, 1],
56+
[4097, 1080],
57+
[1920, 0],
58+
[1920.5, 1080],
59+
])('rejects scale_pad at %sx%s', async (width, height) => {
60+
await expect(runFfmpegOperation('scale_pad', [mediaFile()], { width, height })).rejects.toThrow(
61+
/must be an integer between 16 and 4096|requires width\+height/
62+
)
63+
})
64+
})
65+
66+
describe('runFfmpegOperation abort handling', () => {
67+
it('refuses to start once the signal is already aborted', async () => {
68+
const controller = new AbortController()
69+
controller.abort()
70+
71+
await expect(
72+
runFfmpegOperation('convert', [mediaFile()], { format: 'mp3' }, { signal: controller.signal })
73+
).rejects.toThrow(/aborted/i)
74+
})
75+
})

0 commit comments

Comments
 (0)