fix(engine): isolate WAV staging in a private directory - #3709
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed at 1cbe43dd8f402118e5461139c937700bc44a975c; no blockers.
The exclusive-create change closes the ownership hole at the right boundary (packages/engine/src/services/audioVolumeEnvelope.ts:176-187). writeFileSync(..., { flag: "wx" }) maps to create-plus-exclusive semantics: an existing regular entry or symlink fails before it is opened, so neither the staged entry nor a symlink target can be overwritten. The catch returns false and deliberately does not unlink the colliding path, because this invocation never acquired ownership of it. On success, the existing same-directory renameSync atomically replaces the original and consumes the staging entry.
The collision tests are non-vacuous and cover both objects that matter (packages/engine/src/services/audioVolumeEnvelope.test.ts:63-87): the original WAV stays byte-identical, the existing staging entry stays intact, and a symlink's target stays intact. The successful-path directory census also pins that rename leaves no staging file (:102-118). Windows retains the regular-file proof; the symlink leg is correctly scoped away from its privilege requirement.
Both production callers preserve fallback semantics. The individual-track path leaves bakedEnvelope=false, so base volume/keyframes flow to the existing FFmpeg expression (packages/engine/src/services/audioMixer.ts:1310-1333); the group path does the same when assembling volumeKeyframes (:1485-1510). The surrounding mixer already owns recursive work-directory cleanup on failure and completion (:1390-1405, :1530-1536), so a later write/rename failure cannot create a permanent orphan outside the caller's lifecycle.
CodeQL's js/insecure-temporary-file recommendation requires both nonexistence and inaccessible placement. The new wx provides the first; production WAVs are siblings inside caller-created work directories, which are allocated with mkdtempSync, providing the second without adding a dependency.
CI is still running at this head; initial probes are green, while build, Windows/render regression, and JavaScript CodeQL remain landing gates. Left unmerged as requested.
Verdict: APPROVE
Reasoning: Exclusive creation establishes staging-file ownership before any write, collisions preserve every pre-existing object, and both callers retain their existing audio fallback and cleanup behavior.
— Magi
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed at e115899d40c8f40535086318a98561902d9ef851; no blockers.
This revision now satisfies both halves of CodeQL js/insecure-temporary-file's recommendation. mkdtempSync(join(dirname(wavPath), ".hf-volume-")) atomically creates a private sibling directory (0700 on POSIX), and the fixed audio.wav inside it is still opened with wx (packages/engine/src/services/audioVolumeEnvelope.ts:169-182). The rename remains same-filesystem and atomic.
The ownership matrix is correct:
- read/parse or directory-creation failure:
stagingDiris unset, return false, clean nothing; - write or rename failure after creation: return false, then recursively remove only the directory this call created;
- success: rename consumes the staged WAV, return true, then remove the empty directory;
- cleanup failure: swallowed in the inner cleanup catch, so it cannot replace either the successful bake or the false-return fallback (
:184-195).
The new tests exercise that matrix through the production function. Injected write and rename failures preserve the original WAV and leave no staging entry (packages/engine/src/services/audioVolumeEnvelope.test.ts:62-85); the success/cleanup-failure case proves sibling placement, distinct ownership, 0700 permissions, baked samples, and the retained true result (:87-107). Both individual-track and group callers are unchanged and still interpret false as the existing FFmpeg-envelope fallback.
Local exact-head run: all three new failure/permission cases passed. The full file reported 11/12 here; the only failure is the unchanged real-FFmpeg fixture asserting format tag 3 before calling this function, while this machine's FFmpeg emitted extensible tag 65534. That is environment/binary-specific and outside the diff; current CI uses the supported lane.
Fresh CI and JavaScript CodeQL are still running and remain landing gates. The stale cancelled checks shown by gh pr checks belong to superseded workflow runs, not code failures on this revision. Left unmerged as requested.
Verdict: APPROVE
Reasoning: The staging file now lives in an atomically created private directory, cleanup follows ownership on every exit, atomic replacement and fallback semantics remain intact, and the relevant failure branches are directly pinned.
— Magi
Fixes CodeQL alert #420 and its PR finding #882. WAV volume-envelope staging previously wrote to a random sibling path without guaranteeing private placement or exclusive ownership.
Create a private
mkdtempdirectory beside the WAV, write the staged WAV exclusively inside it, and atomically rename it over the original on the same filesystem. Remove only this invocation's owned directory on success or failure. Cleanup errors preserve the operation result so an already-baked envelope cannot be applied twice through fallback. Audio processing and both mixer callers' existing false-return fallback remain unchanged.Validation: all 12 envelope tests pass, including PCM/float sample accuracy, dense keyframes and a real FFmpeg float-WAV case. New tests verify original WAV preservation and staging removal after partial-write and rename failures, private sibling placement (0700 on POSIX), no staging after success, and a successful result despite cleanup failure. Parser/core builds, engine typecheck, lint/format pass. Both individual-track and group mixer fallback/parent cleanup paths were source-checked.