Repository navigation
fix(skills): stage media downloads in private directories - #3787
Merged
Merged
Conversation
miguel-heygen
approved these changes
Sep 8, 2026
miguel-heygen
left a comment
miguel-heygen
left a comment
Collaborator
There was a problem hiding this comment.
Exact-head security/lifecycle review at 5f6d4cb0a4c1a4b835c03c197adfce1ad865c6f5.
skills/media-use/scripts/lib/heygen-video-provider.mjs:110-125gives the provider clear ownership:mkdtempSyncatomically creates the private directory; every download failure attempts recursive cleanup but preserves the original diagnostic/null contract if cleanup itself fails. Success deliberately skips cleanup and returns the still-readablevideo.mp4; the production resolver copies thatlocalPathonly after the provider returns, matching the previous successful-file lifetime.skills/media-use/scripts/lib/lut-preset-provider.mjs:218-232stages underdirname(fullPath), so validation and the final rename stay on one filesystem. The reserved final path remains the publication owner; download, validation, rename, and staging-cleanup failures all pass throughwithReservedFilerollback, while URL+params failures still enter the existing deterministic fallback.- All four
freezeUrlcall sites are accounted for: the two temporary callers are now private-directory paths, while bothresolve.mjscallers write towithReservedFile's durable exclusive reservation rather than a shared temp name. The planted legacy video and LUT sibling paths therefore have no remaining route intofreezeUrl. - Focused provider tests pass 21/21 at head. Running the head tests against the original two provider implementations produces three failures, including both planted-path discriminators. A separate invalid-remote-LUT probe produced
via=params-fallback, reusedlut_001.cube, validated the final cube, and left only that final file—no staging residue. The manifest check is byte-current.
No blocking findings. The JavaScript CodeQL scan completed successfully with no PR alerts; skills tests, manifest, project-skill checks, format/title, and the applicable regression context are green. Broader build/render jobs are path-filtered as the PR states. The disclosed hostile-ancestor boundary is unchanged.
Verdict: APPROVE
Reasoning: Private atomic staging closes both planted-name paths, cleanup ownership is complete on failures, success retains the generated video contract, and LUT validation/publication/fallback remain correctly ordered.
— Magi
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem and change
Addresses CodeQL #778: the shared freeze helper receives predictable temporary filenames from the HeyGen video and online LUT providers. Another process can preplant those names before a download writes them.
Create a private
mkdtempdirectory per download. HeyGen writesvideo.mp4inside its directory and removes the directory on download failure while preserving the existing null/error behavior. Online LUT downloads stage inside the destination directory, validate the cube, atomically rename to the reserved final path, and remove staging afterward.The shared freeze helper, download limits, returned metadata shape, successful video lifetime, LUT final path/validation/fallback behavior, and offline LUT path remain unchanged. A successful generated video remains available at its returned
localPath, as before. This does not claim to sandbox hostile same-user ancestor renames. No workflow changes; only the media-use manifest fingerprint changes.Validation