Skip to content

fix(producer): isolate assembly scratch directories - #3798

Merged
jrusso1020 merged 1 commit into
mainfrom
fix/security-assemble-temp
Sep 9, 2026
Merged

fix(producer): isolate assembly scratch directories#3798
jrusso1020 merged 1 commit into
mainfrom
fix/security-assemble-temp

Conversation

@jrusso1020

Copy link
Copy Markdown
Collaborator

Distributed assembly used a predictable <output>.assemble-work directory and recursively deleted anything already there. Create a private mkdtemp sibling for each invocation, then retain the existing finally cleanup for that invocation's directory. This addresses CodeQL #408's temporary concat-list path and prevents assembly from removing unrelated data at the old scratch path.

Final output paths, stream-copy/CFR options, audio normalization, provenance, and PNG assembly remain unchanged. Existing directories at the former scratch path are now preserved. The output parent remains a trusted caller-selected directory.

Validation: real FFmpeg assembly tests cover output format/frame count/rate/audio and successful/failing scratch cleanup; POSIX staging permissions are checked as 0700. All 13 assembly tests pass (66 assertions), the two security regressions fail against the baseline, and producer typecheck plus oxlint/oxfmt pass.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head security/lifecycle review at 973db1c5c48f403167f2b8b988aa0d982415eaa3.

  • packages/producer/src/services/distributed/assemble.ts:153-161 creates the output parent first, then atomically allocates a private sibling with mkdtempSync. Production no longer names, probes, deletes, or recreates the legacy <output>.assemble-work path, so pre-existing data there is outside this invocation's ownership.
  • The existing try/finally still encloses every container-format operation after acquisition: single remux, concat-list write/copy, optional CFR, audio normalization, mux, and faststart. Success, FFmpeg failure, typed CFR rejection, audio/mux failure, faststart failure, and abort all reach cleanup of exactly the minted directory. Cleanup failure remains warning-only and cannot replace the primary render result/error.
  • PNG assembly returns before scratch acquisition and is byte-unchanged. Final outputPath, -c copy/CFR args, normalized-audio and provenance paths are unchanged; only scratch children receive the new parent. Concurrent invocations get distinct scratch directories while the disclosed final-output/trusted-parent overwrite boundary remains the same.
  • assemble.test.ts:254-280,326-356 observes the private directory while real FFmpeg is running, checks POSIX 0700, proves the old sentinel survives, and proves both success and concat failure remove only the invocation directory. Both controls fail on the baseline mechanism. Exact-head Producer unit/integration, Build, Typecheck, runtime contract, lint/format, and JavaScript CodeQL are green; the PR scan has no alerts.

Fallow reconciliation: the audit reports 3 complexity findings and 4 clone groups, all introduced:false. The complexity is the pre-existing assemble/mergePngFrameDirs shape; this patch removes an existence/delete branch. All four clone groups are pre-existing FFmpeg test scaffolds, and neither new security regression is a clone source. No suppression or config change was added, so I have no concern with the seven inherited findings for this patch.

No blocking or follow-up findings. Required aggregate Test and Windows render are still running and remain merge gates.

Verdict: APPROVE
Reasoning: Private atomic scratch allocation removes ownership of the predictable path, existing cleanup covers every acquired-resource exit, and the media/final-output contracts are unchanged and regression-tested.

— Magi

@jrusso1020
jrusso1020 merged commit c7891a9 into main Sep 9, 2026
56 checks passed
@jrusso1020
jrusso1020 deleted the fix/security-assemble-temp branch September 9, 2026 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants