From 608c3b50babdec92bb163763ff5c0b8eb7f8f1db Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 20 Aug 2026 15:04:17 -0700 Subject: [PATCH] fix(engine): give the group sub-mix the failure contract, and sanitize its path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding 4 plus the workdir-traversal tail item. **The group loop had no try/catch.** `parseAudioFxChain`, `parseAutomation` and `applyAudioFxChain` were called bare, so a malformed `data-fx-chain` on a BUS — hand-authored, or written by a newer studio carrying an effect id this engine does not know — threw straight out of `processCompositionAudio`. That bypassed the MixResult/`failures[]` shape every caller handles, and skipped `bail()`, so the temp dir leaked with it. The per-element loop has always wrapped the identical calls. Now both do, with the same rule: an `AudioFxRenderError` stays fatal, because substituting the dry signal for a processed one ships a render that sounds plausible and is not what was authored. **The traversal is real, and narrower than it looks.** `group-${groupId}.wav` defuses a bare `../` — the segment is `group-..`, not `..` — but an id holding a slash BEFORE the dots escapes: `a/../../escaped` normalizes to `/../escaped.wav`, outside the tree `bail()`'s rmSync can reach. Verified: without `safePathSegment` the test finds `escaped.wav` sitting beside workDir. `data-audio-group` reaches this file straight from the document; the studio's `GROUP_ID_PATTERN` guards only ids the studio itself mints. Two tests, each verified against a revert of its own fix. engine: 66 files. --- .../src/services/audioMixer.grouping.test.ts | 65 +++++- packages/engine/src/services/audioMixer.ts | 190 +++++++++++------- 2 files changed, 180 insertions(+), 75 deletions(-) diff --git a/packages/engine/src/services/audioMixer.grouping.test.ts b/packages/engine/src/services/audioMixer.grouping.test.ts index 7359a3754..91684bd0e 100644 --- a/packages/engine/src/services/audioMixer.grouping.test.ts +++ b/packages/engine/src/services/audioMixer.grouping.test.ts @@ -1,5 +1,5 @@ import { spawnSync } from "node:child_process"; -import { mkdtempSync, rmSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readdirSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; @@ -401,3 +401,66 @@ describe.skipIf(!HAS_FFMPEG)( }, 60_000, ); + +describe.skipIf(!HAS_FFMPEG)("group sub-mix failure contract", () => { + afterEach(() => { + for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + + // A malformed chain on a BUS threw straight out of processCompositionAudio, + // bypassing the MixResult/failures[] shape the caller handles and skipping + // bail()'s workDir cleanup. The per-element loop has always wrapped the + // identical calls. + it("reports an unparseable group fx-chain as a failure instead of throwing", async () => { + const projectDir = mkdtempSync(join(tmpdir(), "hf-grp-bad-")); + const workDir = mkdtempSync(join(tmpdir(), "hf-grp-bad-work-")); + tempDirs.push(projectDir, workDir); + writeTone(join(projectDir, "a.wav"), 440, 1, 0.4); + + const result = await processCompositionAudio( + [ + { + ...track("a", 1), + groupId: "vo", + groupFxChain: '{"version":1,"nodes":[{"type":"tapestop"}]}', + }, + ], + projectDir, + workDir, + join(projectDir, `bad-${MIXED_AUDIO_FILENAME}`), + 1, + ); + + expect(result.success).toBe(false); + const failure = result.failures?.find((f) => f.elementId === "vo"); + expect(failure?.stage).toBe("mix"); + expect(failure?.detail).toContain("tapestop"); + }); + + // `data-audio-group` arrives unvalidated from the document. `group-${id}.wav` + // defuses a bare `../` (the segment is `group-..`, not `..`), but an id + // holding a slash BEFORE the dots does escape: `a/../../x` normalizes to + // `/../x.wav`, outside the tree `bail()`'s rmSync can reach. + it("keeps a traversal-shaped group id inside the work directory", async () => { + const parent = mkdtempSync(join(tmpdir(), "hf-grp-parent-")); + tempDirs.push(parent); + const projectDir = join(parent, "project"); + const workDir = join(parent, "work"); + mkdirSync(projectDir); + mkdirSync(workDir); + writeTone(join(projectDir, "a.wav"), 440, 1, 0.4); + + const result = await processCompositionAudio( + [{ ...track("a", 1), groupId: "a/../../escaped" }], + projectDir, + workDir, + join(projectDir, `esc-${MIXED_AUDIO_FILENAME}`), + 1, + ); + + expect(result.success).toBe(true); + // workDir is cleaned up on success, so what matters is what is left BESIDE + // it: an escaped would survive there. Only the project remains. + expect(readdirSync(parent).sort()).toEqual(["project"]); + }); +}); diff --git a/packages/engine/src/services/audioMixer.ts b/packages/engine/src/services/audioMixer.ts index df93dc62f..ba46a812e 100644 --- a/packages/engine/src/services/audioMixer.ts +++ b/packages/engine/src/services/audioMixer.ts @@ -72,6 +72,23 @@ function clampVolume(volume: number): number { return clampAudioGain(volume); } +/** + * An author-controlled id, made safe to put in a filename. + * + * `data-audio-group` reaches this file straight from the document — the + * studio's `GROUP_ID_PATTERN` guards only ids the studio itself mints, and a + * hand-authored or agent-written one is unvalidated. Interpolated raw it could + * carry `/` or `..`, and `mkdirSync(recursive)` inside ffmpeg's own path + * handling would then write outside `workDir`, where `bail()`'s `rmSync` never + * cleans it up. Everything outside [A-Za-z0-9_-] collapses to `_`, and an id + * that sanitizes to nothing gets a stable positional fallback rather than an + * empty segment that two groups would share. + */ +function safePathSegment(id: string, fallbackIndex: number): string { + const cleaned = id.replace(/[^A-Za-z0-9_-]/g, "_"); + return cleaned.length > 0 ? cleaned : `group-${fallbackIndex}`; +} + function formatFilterNumber(value: number): string { return Number(value.toFixed(6)).toString(); } @@ -1336,92 +1353,117 @@ export async function processCompositionAudio( // own, and members are already delayed to their composition positions // inside the sub-mix, so the group WAV's t=0 IS composition time. const groupsDegradedAutomation: string[] = []; + let groupIndex = -1; for (const [groupId, memberTracks] of groupTracks) { + groupIndex += 1; const meta = groupMeta.get(groupId); if (!meta) continue; - const groupWavPath = join(workDir, `group-${groupId}.wav`); - const subMix = await mixGroupMembers( - memberTracks, - groupWavPath, - totalDuration, - effectiveSignal, - config, - ); - if (!subMix.success) { + const pathId = safePathSegment(groupId, groupIndex); + const groupWavPath = join(workDir, `group-${pathId}.wav`); + // Same contract the per-element loop above gives: a malformed `data-fx-chain` + // or `data-automation` on a BUS threw straight out of processCompositionAudio, + // bypassing the MixResult/failures[] shape the caller handles and skipping + // `bail()`, so the temp dir leaked too. An AudioFxRenderError stays fatal + // for the same reason it is fatal per element: shipping the dry signal in + // place of a processed one sounds plausible and is not what was authored. + try { + const subMix = await mixGroupMembers( + memberTracks, + groupWavPath, + totalDuration, + effectiveSignal, + config, + ); + if (!subMix.success) { + failures.push({ + stage: "mix", + reason: "ffmpeg_failed", + owner: "system", + retryable: false, + elementId: groupId, + detail: boundedDetail( + `Group sub-mix failed for group ${groupId}: ${subMix.error ?? "unknown"}`, + ), + }); + continue; + } + if (subMix.degradedAutomation) groupsDegradedAutomation.push(groupId); + + // Composition-time automation (offset 0, duration totalDuration) — same + // resolve/lane/bake path a member uses, just anchored at the group clock + // instead of a clip's own start. + const automation = meta.automation + ? resolveAutomation( + parseAutomation(meta.automation), + meta.fxChain ? parseAudioFxChain(meta.fxChain) : undefined, + ) + : null; + const laneKeyframes = automation ? volumeLaneKeyframes(automation, 0, totalDuration) : null; + const envelope = + laneKeyframes && laneKeyframes.length > 0 + ? { keyframes: laneKeyframes, trackStart: 0, baseVolume: meta.volume } + : null; + + let groupSrcPath = groupWavPath; + let bakedEnvelope = false; + if (meta.fxChain) { + const chain = parseAudioFxChain(meta.fxChain); + const fxResult = await applyAudioFxChain( + groupSrcPath, + chain, + join(workDir, `group-${pathId}-fx.wav`), + { + trackId: groupId, + signal: effectiveSignal, + ...(automation ? { automation } : {}), + ...(envelope ? { envelope } : {}), + }, + ); + groupSrcPath = fxResult.path; + bakedEnvelope = fxResult.envelopeBaked; + } + if (envelope && !bakedEnvelope) { + bakedEnvelope = applyVolumeEnvelopeToWav( + groupSrcPath, + envelope.keyframes, + envelope.trackStart, + envelope.baseVolume, + ); + } + + // `end` is already the render's totalDuration, so the outer mix's own + // atrim-to-totalDuration clips this track exactly the same way it would + // an ungrouped one whose clip ran to the end of the render — a group's FX + // tail gets the same "never past the end of the video" treatment member + // tails get, for free, with no separate tailSeconds bookkeeping needed. + tracks.push({ + id: groupId, + srcPath: groupSrcPath, + start: 0, + end: totalDuration, + mediaStart: 0, + duration: totalDuration, + volume: bakedEnvelope ? 1.0 : meta.volume, + // Same fallback an ungrouped track gets: when the envelope could not be + // baked into the samples, hand the keyframes to the outer mix's volume + // expression instead of dropping the group's automation on the floor. + ...(bakedEnvelope || !laneKeyframes?.length ? {} : { volumeKeyframes: laneKeyframes }), + }); + } catch (err: unknown) { + if (err instanceof AudioFxRenderError) throw err; failures.push({ stage: "mix", - reason: "ffmpeg_failed", + reason: "internal", owner: "system", retryable: false, elementId: groupId, detail: boundedDetail( - `Group sub-mix failed for group ${groupId}: ${subMix.error ?? "unknown"}`, + `Audio processing failed for group ${groupId}: ${ + err instanceof Error ? err.message : String(err) + }`, ), }); - continue; } - if (subMix.degradedAutomation) groupsDegradedAutomation.push(groupId); - - // Composition-time automation (offset 0, duration totalDuration) — same - // resolve/lane/bake path a member uses, just anchored at the group clock - // instead of a clip's own start. - const automation = meta.automation - ? resolveAutomation( - parseAutomation(meta.automation), - meta.fxChain ? parseAudioFxChain(meta.fxChain) : undefined, - ) - : null; - const laneKeyframes = automation ? volumeLaneKeyframes(automation, 0, totalDuration) : null; - const envelope = - laneKeyframes && laneKeyframes.length > 0 - ? { keyframes: laneKeyframes, trackStart: 0, baseVolume: meta.volume } - : null; - - let groupSrcPath = groupWavPath; - let bakedEnvelope = false; - if (meta.fxChain) { - const chain = parseAudioFxChain(meta.fxChain); - const fxResult = await applyAudioFxChain( - groupSrcPath, - chain, - join(workDir, `group-${groupId}-fx.wav`), - { - trackId: groupId, - signal: effectiveSignal, - ...(automation ? { automation } : {}), - ...(envelope ? { envelope } : {}), - }, - ); - groupSrcPath = fxResult.path; - bakedEnvelope = fxResult.envelopeBaked; - } - if (envelope && !bakedEnvelope) { - bakedEnvelope = applyVolumeEnvelopeToWav( - groupSrcPath, - envelope.keyframes, - envelope.trackStart, - envelope.baseVolume, - ); - } - - // `end` is already the render's totalDuration, so the outer mix's own - // atrim-to-totalDuration clips this track exactly the same way it would - // an ungrouped one whose clip ran to the end of the render — a group's FX - // tail gets the same "never past the end of the video" treatment member - // tails get, for free, with no separate tailSeconds bookkeeping needed. - tracks.push({ - id: groupId, - srcPath: groupSrcPath, - start: 0, - end: totalDuration, - mediaStart: 0, - duration: totalDuration, - volume: bakedEnvelope ? 1.0 : meta.volume, - // Same fallback an ungrouped track gets: when the envelope could not be - // baked into the samples, hand the keyframes to the outer mix's volume - // expression instead of dropping the group's automation on the floor. - ...(bakedEnvelope || !laneKeyframes?.length ? {} : { volumeKeyframes: laneKeyframes }), - }); } if (failures.length > 0) return bail();