diff --git a/src/ffmpeg/escape.ts b/src/ffmpeg/escape.ts index 454d112..751c986 100644 --- a/src/ffmpeg/escape.ts +++ b/src/ffmpeg/escape.ts @@ -1,28 +1,20 @@ /** - * Quote a value for safe use as a filter option value in an ffmpeg filter - * graph. Wraps the value in single quotes and escapes internal backslashes - * and single quotes per ffmpeg's filter escape rules. + * Escape a filter option value through FFmpeg's option and filtergraph parsers. + * The graph parser consumes the outer quotes first, so option-level escapes + * must survive it. In particular, a Windows drive colon still separates options + * without its own escape, even inside an outer quoted filtergraph value. * - * This handles the FIRST level of escaping — filter option values — which - * is enough for paths, color names, numeric expressions, and short text - * snippets that go directly into a `key=value` pair. - * - * Some filter options (notably `drawtext` `text=`) interpret their content - * as a SECOND-level expression where additional escaping rules apply. - * For user-provided text, route the content through `textfile=` - * instead of inlining; this function quotes the path safely while keeping - * the text content itself out of the filter graph entirely. - * - * See: https://ffmpeg.org/ffmpeg-filters.html#Filtergraph-syntax-1 - * - * @example - * quoteFilterArg("don't") // "'don\\'t'" - * quoteFilterArg('a:b,c[d]') // "'a:b,c[d]'" - * quoteFilterArg('/tmp/x.txt') // "'/tmp/x.txt'" + * These values are passed in an argument array, not through a shell. User text + * should still go through textfile rather than drawtext's text expansion. + * See https://ffmpeg.org/ffmpeg-filters.html#Notes-on-filtergraph-escaping */ export function quoteFilterArg(value: string): string { - // Order matters: escape backslashes first, otherwise the backslash we add - // to escape a single quote would itself get re-doubled on a second pass. - const escaped = value.replace(/\\/g, '\\\\').replace(/'/g, "\\'"); - return `'${escaped}'`; + const optionValue = value.replace(/[\\':]/g, (char) => `\\${char}`); + return `'${optionValue.replace(/'/g, "'\\''")}'`; +} + +/** Quote a filename for the concat demuxer's single parsing layer. */ +export function quoteConcatPath(value: string): string { + if (/[\r\n\0]/.test(value)) throw new Error('Concat paths must not contain line breaks or NUL'); + return `'${value.replace(/'/g, "'\\''")}'`; } diff --git a/src/tools/concat.ts b/src/tools/concat.ts index 8e6f3b8..f3afce7 100644 --- a/src/tools/concat.ts +++ b/src/tools/concat.ts @@ -3,7 +3,7 @@ import { unlink, writeFile } from 'node:fs/promises'; import { resolve } from 'node:path'; import { z } from 'zod'; import { buildConcatArgs } from '../ffmpeg/args/concat.js'; -import { quoteFilterArg } from '../ffmpeg/escape.js'; +import { quoteConcatPath } from '../ffmpeg/escape.js'; import { runFfmpeg } from '../ffmpeg/run.js'; import { ensureWorkspace, getWorkspace, newOutputPath, resolveInput } from '../workspace.js'; @@ -28,7 +28,7 @@ export interface ConcatResult { * trailing newline) and small mistakes show up only at ffmpeg-runtime. */ export function buildConcatListContent(absolutePaths: string[]): string { - return `${absolutePaths.map((path) => `file ${quoteFilterArg(path)}`).join('\n')}\n`; + return `${absolutePaths.map((path) => `file ${quoteConcatPath(path)}`).join('\n')}\n`; } export async function concat(input: ConcatInputType): Promise { diff --git a/tests/add-text.test.ts b/tests/add-text.test.ts index 129a213..3eb984b 100644 --- a/tests/add-text.test.ts +++ b/tests/add-text.test.ts @@ -8,26 +8,24 @@ describe('quoteFilterArg', () => { expect(quoteFilterArg('hello')).toBe("'hello'"); }); - it('escapes internal single quotes', () => { - expect(quoteFilterArg("don't")).toBe("'don\\'t'"); + it('preserves a quote through both parser layers', () => { + expect(quoteFilterArg("don't")).toBe(String.raw`'don\'\''t'`); }); - it('escapes backslashes', () => { - expect(quoteFilterArg('a\\b')).toBe("'a\\\\b'"); + it('preserves backslashes through both parser layers', () => { + expect(quoteFilterArg(String.raw`a\b`)).toBe(String.raw`'a\\b'`); }); - it('escapes backslash before single quote (backslash escaped first)', () => { - // Input "\\'" (literal backslash then single quote) should become "\\\\\\'" - // — the backslash doubles to \\, and the quote becomes \'. - expect(quoteFilterArg("\\'")).toBe("'\\\\\\''"); + it('preserves a backslash immediately before a quote', () => { + expect(quoteFilterArg(String.raw`\'`)).toBe(String.raw`'\\\'\'''`); }); it('handles empty string', () => { expect(quoteFilterArg('')).toBe("''"); }); - it('leaves colons, commas, brackets alone (single-quoting handles them)', () => { - expect(quoteFilterArg('a:b,c[d];e')).toBe("'a:b,c[d];e'"); + it('protects option colons as well as filtergraph delimiters', () => { + expect(quoteFilterArg('a:b,c[d];e')).toBe(String.raw`'a\:b,c[d];e'`); }); it('handles unicode without modification', () => { @@ -38,17 +36,14 @@ describe('quoteFilterArg', () => { expect(quoteFilterArg('/tmp/text-abc.txt')).toBe("'/tmp/text-abc.txt'"); }); - it('handles macOS /var/folders paths', () => { - const p = '/var/folders/dn/vk2rnbvx7k98/T/makemyclip-editor/add-text-123.txt'; - expect(quoteFilterArg(p)).toBe(`'${p}'`); + it('keeps Windows drive colons and punctuation inside the filename', () => { + expect(quoteFilterArg(String.raw`C:\Users\Agent's work, [draft];\notes.txt`)).toBe( + String.raw`'C\:\\Users\\Agent\'\''s work, [draft];\\notes.txt'`, + ); }); - it('round-trips through a simulated parser', () => { - const original = "tricky: 'value' with\\backslash and: more, stuff"; - const quoted = quoteFilterArg(original); - // Simulate ffmpeg: strip outer quotes, unescape \' then \\ in that order - const unquoted = quoted.slice(1, -1).replace(/\\'/g, "'").replace(/\\\\/g, '\\'); - expect(unquoted).toBe(original); + it('preserves leading and trailing whitespace', () => { + expect(quoteFilterArg(' spaced name ')).toBe("' spaced name '"); }); }); diff --git a/tests/compile.test.ts b/tests/compile.test.ts index 75522f0..74b2809 100644 --- a/tests/compile.test.ts +++ b/tests/compile.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from 'vitest'; +import { quoteFilterArg } from '../src/ffmpeg/escape.js'; import { type CompileContext, CompileError, compileTimeline } from '../src/timeline/compile.js'; import { emptyComposition } from '../src/timeline/composition.js'; import { applyOps, colorClip, mediaClip, textClip, videoTrack } from '../src/timeline/ops.js'; @@ -117,7 +118,7 @@ describe('compileTimeline — generated clips', () => { expect(step?.textFiles[0]?.content).toBe("It's a title: colons & commas"); const fc = step?.args[step.args.indexOf('-filter_complex') + 1] ?? ''; expect(fc).toContain('drawtext='); - expect(fc).toContain(`textfile='${step?.textFiles[0]?.path}'`); + expect(fc).toContain(`textfile=${quoteFilterArg(step?.textFiles[0]?.path ?? '')}`); // The raw text must NOT be inlined in the filter graph. expect(fc).not.toContain('a title: colons'); }); diff --git a/tests/concat.test.ts b/tests/concat.test.ts index 97ede13..1d5889a 100644 --- a/tests/concat.test.ts +++ b/tests/concat.test.ts @@ -38,12 +38,20 @@ describe('buildConcatListContent', () => { it('escapes single quotes in paths', () => { const content = buildConcatListContent(["/tmp/it's a file.mp4"]); - expect(content).toBe("file '/tmp/it\\'s a file.mp4'\n"); + expect(content).toBe("file '/tmp/it'\\''s a file.mp4'\n"); }); - it('escapes backslashes in paths', () => { + it('keeps backslashes literal inside a quoted concat path', () => { const content = buildConcatListContent(['C:\\videos\\clip.mp4']); - expect(content).toBe("file 'C:\\\\videos\\\\clip.mp4'\n"); + expect(content).toBe("file 'C:\\videos\\clip.mp4'\n"); + }); + + it.each([ + 'line\nbreak.mp4', + 'carriage\rreturn.mp4', + 'nul\0byte.mp4', + ])('rejects a path that cannot be represented as one list line: %s', (path) => { + expect(() => buildConcatListContent([path])).toThrow(/line breaks or NUL/); }); it('handles unicode paths', () => { diff --git a/tests/integration/filter-escape.test.ts b/tests/integration/filter-escape.test.ts new file mode 100644 index 0000000..326dc7a --- /dev/null +++ b/tests/integration/filter-escape.test.ts @@ -0,0 +1,83 @@ +import { mkdtemp, rmdir, unlink, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { describe, it } from 'vitest'; +import { buildAddTextArgs } from '../../src/ffmpeg/args/add-text.js'; +import { buildConcatArgs } from '../../src/ffmpeg/args/concat.js'; +import { runFfmpeg } from '../../src/ffmpeg/run.js'; +import { buildConcatListContent } from '../../src/tools/concat.js'; + +// The real parser catches errors that a string-unescaping simulation misses. +describe.skipIf(process.env.CI !== 'true')('filter option escaping with FFmpeg', () => { + it.each([ + 'plain note.txt', + "O'Brien, [notes]; v1.txt", + process.platform === 'win32' ? 'drive path.txt' : 'drive:path.txt', + ])('reads the exact textfile %s', async (name) => { + const dir = await mkdtemp(join(tmpdir(), 'makemyclip-escape-')); + const textfile = join(dir, name); + try { + await writeFile(textfile, 'Visible text', 'utf8'); + const filter = buildAddTextArgs({ + input: 'unused', + output: 'unused', + textfile, + position: 'center', + startSec: 0, + endSec: 1, + fontSize: 16, + box: false, + })[4]; + if (!filter) throw new Error('Missing drawtext filter'); + await runFfmpeg([ + '-hide_banner', + '-loglevel', + 'error', + '-f', + 'lavfi', + '-i', + 'color=c=black:s=96x64:r=10:d=0.2', + '-vf', + filter, + '-frames:v', + '1', + '-f', + 'null', + '-', + ]); + } finally { + await unlink(textfile); + await rmdir(dir); + } + }); + + it('uses concat-list quoting without filter-option escapes', async () => { + const dir = await mkdtemp(join(tmpdir(), 'makemyclip-escape-')); + const input = join(dir, "O'Brien, [clip];.mp4"); + const listFile = join(dir, 'list.txt'); + const output = join(dir, 'joined.mp4'); + try { + await runFfmpeg([ + '-hide_banner', + '-loglevel', + 'error', + '-f', + 'lavfi', + '-i', + 'color=c=black:s=96x64:r=10:d=0.2', + '-c:v', + 'libx264', + '-pix_fmt', + 'yuv420p', + input, + ]); + await writeFile(listFile, buildConcatListContent([input, input]), 'utf8'); + await runFfmpeg(buildConcatArgs({ listFile, output })); + } finally { + for (const file of [input, listFile, output]) { + await unlink(file).catch(() => undefined); + } + await rmdir(dir); + } + }); +});