From efe2404cbdeb86111d78a2f555aacf44b71f15fe Mon Sep 17 00:00:00 2001 From: James Russo Date: Tue, 8 Sep 2026 19:29:26 -0400 Subject: [PATCH] fix(skills): stage media downloads in private directories (#3787) --- skills-manifest.json | 2 +- .../scripts/lib/heygen-video-provider.mjs | 13 ++- .../lib/heygen-video-provider.test.mjs | 95 +++++++++++++++++-- .../scripts/lib/lut-preset-provider.mjs | 11 ++- .../scripts/lib/lut-preset-provider.test.mjs | 25 ++++- 5 files changed, 131 insertions(+), 15 deletions(-) diff --git a/skills-manifest.json b/skills-manifest.json index ceab4acf60..e34115da7d 100644 --- a/skills-manifest.json +++ b/skills-manifest.json @@ -50,7 +50,7 @@ "files": 12 }, "media-use": { - "hash": "a397849a32047f7e", + "hash": "591daf2adb78e1cd", "files": 156 }, "motion-graphics": { diff --git a/skills/media-use/scripts/lib/heygen-video-provider.mjs b/skills/media-use/scripts/lib/heygen-video-provider.mjs index 799ac0442e..b05c6d8ccb 100644 --- a/skills/media-use/scripts/lib/heygen-video-provider.mjs +++ b/skills/media-use/scripts/lib/heygen-video-provider.mjs @@ -1,4 +1,5 @@ import { execFileSync } from "node:child_process"; +import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { freezeUrl } from "./freeze.mjs"; @@ -106,10 +107,20 @@ export async function heygenVideoGenerate(intent, ctx) { return null; } - const tmpPath = join(tmpdir(), `media-use-heygen-video-${process.pid}-${Date.now()}.mp4`); + let tmpDir; + let tmpPath; try { + tmpDir = mkdtempSync(join(tmpdir(), "media-use-heygen-video-")); + tmpPath = join(tmpDir, "video.mp4"); await freezeUrl(videoUrl, tmpPath); } catch (err) { + if (tmpDir) { + try { + rmSync(tmpDir, { recursive: true, force: true }); + } catch { + // Preserve the download failure if temporary-directory cleanup fails. + } + } console.error(`media-use: heygen video download failed: ${err.message}`); return null; } diff --git a/skills/media-use/scripts/lib/heygen-video-provider.test.mjs b/skills/media-use/scripts/lib/heygen-video-provider.test.mjs index d258d4c9eb..d1b87fd35f 100644 --- a/skills/media-use/scripts/lib/heygen-video-provider.test.mjs +++ b/skills/media-use/scripts/lib/heygen-video-provider.test.mjs @@ -1,12 +1,32 @@ import { strict as assert } from "node:assert"; -import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + chmodSync, + mkdtempSync, + readFileSync, + rmSync, + statSync, + symlinkSync, + readdirSync, + writeFileSync, +} from "node:fs"; import http from "node:http"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { basename, dirname, join } from "node:path"; import { test } from "node:test"; import { HEYGEN_NOT_AUTHENTICATED_MESSAGE } from "./heygen-cli.mjs"; import { AVATAR_VIDEO_SIGNIN_MESSAGE } from "./heygen-video-provider.mjs"; +function cleanupDownload(localPath) { + if ( + basename(localPath) === "video.mp4" && + basename(dirname(localPath)).startsWith("media-use-heygen-video-") + ) { + rmSync(dirname(localPath), { recursive: true, force: true }); + } else { + rmSync(localPath, { force: true }); + } +} + const VIDEO_FIXTURE = Buffer.from("tiny heygen video fixture"); let importCount = 0; @@ -148,7 +168,10 @@ test("downloads a generated avatar video and returns the generated MP4 result", }); assert.ok(result); assert.equal(join(tmpdir(), result.localPath.slice(tmpdir().length + 1)), result.localPath); - assert.match(result.localPath, /media-use-heygen-video-\d+-\d+\.mp4$/); + assert.match(result.localPath, /media-use-heygen-video-[^/\\]+[/\\]video\.mp4$/); + if (process.platform !== "win32") { + assert.equal(statSync(dirname(result.localPath)).mode & 0o777, 0o700); + } assert.deepEqual(result, { localPath: result.localPath, ext: ".mp4", @@ -163,7 +186,7 @@ test("downloads a generated avatar video and returns the generated MP4 result", }, ); } finally { - if (localPath) rmSync(localPath, { force: true }); + if (localPath) cleanupDownload(localPath); await closeServer(server); } }); @@ -190,7 +213,7 @@ test("tags video creation but not avatar or voice discovery", async () => { }, ); } finally { - if (localPath) rmSync(localPath, { force: true }); + if (localPath) cleanupDownload(localPath); await closeServer(server); } }); @@ -221,7 +244,7 @@ test("uses explicit avatar and voice overrides without discovery", async () => { }, ); } finally { - if (localPath) rmSync(localPath, { force: true }); + if (localPath) cleanupDownload(localPath); await closeServer(server); } }); @@ -246,7 +269,7 @@ test("caches discovered avatar and voice IDs for the process", async () => { }, ); } finally { - for (const localPath of localPaths) rmSync(localPath, { force: true }); + for (const localPath of localPaths) cleanupDownload(localPath); await closeServer(server); } }); @@ -342,3 +365,61 @@ test("download failure after a successful create returns null and logs a diagnos await closeServer(server); } }); + +test("uses private unique downloads even when time is fixed and the old name is planted", async (t) => { + const root = mkdtempSync(join(tmpdir(), "hf-video-temp-test-")); + const previousTmpdir = process.env.TMPDIR; + process.env.TMPDIR = root; + t.mock.method(Date, "now", () => 123456); + t.mock.method(globalThis, "fetch", async () => ({ + ok: true, + headers: { get: () => "4" }, + body: [Buffer.from("new!")], + })); + try { + const victim = join(root, "victim"); + writeFileSync(victim, "unchanged"); + symlinkSync(victim, join(root, `media-use-heygen-video-${process.pid}-123456.mp4`)); + await withFakeHeygen( + { response: JSON.stringify({ data: { video_url: "https://example.invalid/video.mp4" } }) }, + async () => { + const generate = await freshGenerate(); + const first = await generate("First", { avatarId: "a", voiceId: "v" }); + const second = await generate("Second", { avatarId: "a", voiceId: "v" }); + assert.ok(first && second); + assert.notEqual(first.localPath, second.localPath); + assert.equal(readFileSync(first.localPath, "utf8"), "new!"); + assert.equal(readFileSync(second.localPath, "utf8"), "new!"); + assert.equal(readFileSync(victim, "utf8"), "unchanged"); + }, + ); + } finally { + if (previousTmpdir === undefined) delete process.env.TMPDIR; + else process.env.TMPDIR = previousTmpdir; + rmSync(root, { recursive: true, force: true }); + } +}); + +test("removes private download staging on failure while returning null", async (t) => { + const root = mkdtempSync(join(tmpdir(), "hf-video-temp-fail-")); + const previousTmpdir = process.env.TMPDIR; + process.env.TMPDIR = root; + t.mock.method(globalThis, "fetch", async () => { + throw new Error("download failed"); + }); + t.mock.method(console, "error", () => {}); + try { + await withFakeHeygen( + { response: JSON.stringify({ data: { video_url: "https://example.invalid/video.mp4" } }) }, + async () => { + const generate = await freshGenerate(); + assert.equal(await generate("Failure", { avatarId: "a", voiceId: "v" }), null); + }, + ); + assert.deepEqual(readdirSync(root), []); + } finally { + if (previousTmpdir === undefined) delete process.env.TMPDIR; + else process.env.TMPDIR = previousTmpdir; + rmSync(root, { recursive: true, force: true }); + } +}); diff --git a/skills/media-use/scripts/lib/lut-preset-provider.mjs b/skills/media-use/scripts/lib/lut-preset-provider.mjs index 51c600b38c..a6c907accc 100644 --- a/skills/media-use/scripts/lib/lut-preset-provider.mjs +++ b/skills/media-use/scripts/lib/lut-preset-provider.mjs @@ -1,5 +1,5 @@ -import { existsSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs"; -import { join } from "node:path"; +import { existsSync, mkdtempSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs"; +import { dirname, join } from "node:path"; import { withReservedFile, withReservedFileSync } from "./manifest.mjs"; import { freezeUrl } from "./freeze.mjs"; import { tokenOverlap } from "./match.mjs"; @@ -216,9 +216,10 @@ export async function freezeLibraryLut(match, { projectDir, type, localOnly = fa type, ".cube", async ({ id, localPath, fullPath }) => { - const tmpPath = `${fullPath}.tmp`; + const tmpDir = mkdtempSync(join(dirname(fullPath), ".lut-download-")); + const tmpPath = join(tmpDir, "download.cube"); try { - // Download + validate at a .tmp path, then atomically rename. A crash + // Download + validate in a private directory, then atomically rename. A crash // (SIGKILL/OOM) between write and validate can't orphan an invalid .cube // at the final path — only a validated cube is ever renamed into place. await freezeUrl(match.url, tmpPath); @@ -226,7 +227,7 @@ export async function freezeLibraryLut(match, { projectDir, type, localOnly = fa renameSync(tmpPath, fullPath); return libraryRecord(match, { id, localPath, fullPath, via: "url" }); } finally { - rmSync(tmpPath, { force: true }); + rmSync(tmpDir, { recursive: true, force: true }); } }, ); diff --git a/skills/media-use/scripts/lib/lut-preset-provider.test.mjs b/skills/media-use/scripts/lib/lut-preset-provider.test.mjs index 1d4cd9f8b1..7cfcd209b1 100644 --- a/skills/media-use/scripts/lib/lut-preset-provider.test.mjs +++ b/skills/media-use/scripts/lib/lut-preset-provider.test.mjs @@ -1,5 +1,14 @@ import { strict as assert } from "node:assert"; -import { mkdtempSync, rmSync, existsSync, readFileSync, readdirSync } from "node:fs"; +import { + mkdtempSync, + rmSync, + existsSync, + readFileSync, + readdirSync, + statSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; import { test } from "node:test"; @@ -127,6 +136,15 @@ test("url library entries respect localOnly and freeze through fetch", async () globalThis.fetch = async (url) => { fetchCalls++; assert.equal(url, match.url); + const directory = join(projectDir, ".media/luts"); + const staging = readdirSync(directory).filter((name) => name.startsWith(".lut-download-")); + assert.equal(staging.length, 1); + if (process.platform !== "win32") { + assert.equal(statSync(join(directory, staging[0])).mode & 0o777, 0o700); + } + const victim = join(projectDir, "victim.cube"); + writeFileSync(victim, "unchanged"); + symlinkSync(victim, join(directory, "lut_001.cube.tmp")); return { ok: true, headers: { get: () => String(body.length) }, @@ -139,6 +157,11 @@ test("url library entries respect localOnly and freeze through fetch", async () assert.match(frozen.localPath, /^\.media\/luts\/lut_001\.cube$/); assert.equal(validateCubeFile(join(projectDir, frozen.localPath)).ok, true); assert.equal(frozen.metadata.provenance.via, "url"); + assert.equal(readFileSync(join(projectDir, "victim.cube"), "utf8"), "unchanged"); + assert.deepEqual(readdirSync(join(projectDir, ".media/luts")).sort(), [ + "lut_001.cube", + "lut_001.cube.tmp", + ]); } finally { globalThis.fetch = originalFetch; rmSync(projectDir, { recursive: true, force: true });