fix(producer): tighten resource lifecycle and harden file server#371
fix(producer): tighten resource lifecycle and harden file server#371vanceingalls merged 5 commits intomainfrom
Conversation
cdcd0fd to
f198969
Compare
2e06ff0 to
db58e11
Compare
f198969 to
19698fd
Compare
db58e11 to
efd8422
Compare
19698fd to
c4b1a1c
Compare
| * | ||
| * Exported for unit tests; not part of the public package surface. | ||
| */ | ||
| export function isPathInside(child: string, parent: string): boolean { |
There was a problem hiding this comment.
resolve() only normalizes the lexical path; it does not resolve symlinks. Because the later statSync(candidate).isFile() follows symlinks, a symlink inside projectDir or compiledDir that points outside the allowed root will still pass this check and get served. I reproduced this locally with a symlink-to-outside file, so this guard still needs a realpath-based containment check before we can treat the traversal issue as fixed.
There was a problem hiding this comment.
Addressed. isPathInside now takes a resolveSymlinks option that uses realpathSync.native() when both sides exist, and the two app.get call sites in fileServer.ts pass resolveSymlinks: true. Test coverage in fileServer.test.ts exercises the symlink-escape case (expect(isPathInside(symlinkPath, rootDir, { resolveSymlinks: true })).toBe(false)) and Windows path semantics via the pathModule: path.win32 injection so the cross-platform pin runs on Linux/macOS CI as well. Thanks for catching this — the original lexical-only check was insufficient.
fd205f3 to
658ed49
Compare
a31369c to
19e5afc
Compare
Address jrusso1020's review on PR #371: add a path.win32 test suite that exercises isPathInside on Linux/macOS CI to catch accidental Unix-only assumptions (e.g. only splitting on "/") that would silently regress for Windows users. - isPathInside now accepts an optional `pathModule` (defaults to node:path) so tests can inject path.win32 / path.posix without changing call sites. - New describe block covers equality, direct/deep children, sibling-prefix rejection, traversal escapes, trailing-backslash normalization, and cross-drive rejection.
658ed49 to
4fd6596
Compare
Address jrusso1020's review on PR #371: add a path.win32 test suite that exercises isPathInside on Linux/macOS CI to catch accidental Unix-only assumptions (e.g. only splitting on "/") that would silently regress for Windows users. - isPathInside now accepts an optional `pathModule` (defaults to node:path) so tests can inject path.win32 / path.posix without changing call sites. - New describe block covers equality, direct/deep children, sibling-prefix rejection, traversal escapes, trailing-backslash normalization, and cross-drive rejection.
2f1942d to
7b268a9
Compare
Address jrusso1020's review on PR #371: add a path.win32 test suite that exercises isPathInside on Linux/macOS CI to catch accidental Unix-only assumptions (e.g. only splitting on "/") that would silently regress for Windows users. - isPathInside now accepts an optional `pathModule` (defaults to node:path) so tests can inject path.win32 / path.posix without changing call sites. - New describe block covers equality, direct/deep children, sibling-prefix rejection, traversal escapes, trailing-backslash normalization, and cross-drive rejection.
4fd6596 to
0947ad8
Compare
Address jrusso1020's review on PR #371: add a path.win32 test suite that exercises isPathInside on Linux/macOS CI to catch accidental Unix-only assumptions (e.g. only splitting on "/") that would silently regress for Windows users. - isPathInside now accepts an optional `pathModule` (defaults to node:path) so tests can inject path.win32 / path.posix without changing call sites. - New describe block covers equality, direct/deep children, sibling-prefix rejection, traversal escapes, trailing-backslash normalization, and cross-drive rejection.
… transfer
Chunk 3 of HDR follow-ups. Three independent fixes that share a common
thread: HDR config flowing correctly from the EngineConfig down through
the encoders.
3A. chunkEncoder respects options.hdr (BT.2020 + mastering metadata)
Previously buildEncoderArgs hard-coded BT.709 color tags and the
bt709 VUI block in -x265-params, even when callers passed an HDR
EncoderOptions. Today this is harmless because renderOrchestrator
routes native-HDR content to streamingEncoder and only feeds
chunkEncoder sRGB Chrome screenshots — but the contract was a lie.
Now: when options.hdr is set, the libx265 software path emits
bt2020nc + the matching transfer (smpte2084 for PQ,
arib-std-b67 for HLG) at the codec level *and* embeds
master-display + max-cll SEI in -x265-params via
getHdrEncoderColorParams. libx264 still tags BT.709 inside
-x264-params (libx264 has no HDR support) but the codec-level color
flags flip so the container describes pixels truthfully. GPU
H.265 (nvenc/videotoolbox/qsv/vaapi) gets the BT.2020 tags but no
-x265-params block, so static mastering metadata is omitted —
acceptable for previews, not HDR-aware delivery.
3B. convertSdrToHdr accepts a target transfer
videoFrameExtractor.convertSdrToHdr was hard-coded to
transfer=arib-std-b67 (HLG) regardless of the surrounding
composition's dominant transfer. extractAllVideoFrames now calls
analyzeCompositionHdr first, then passes the dominant transfer
("pq" or "hlg") into convertSdrToHdr so an SDR clip mixed into a PQ
timeline gets converted with smpte2084, not arib-std-b67.
3C. EngineConfig.hdr type matches its declared shape
The IIFE for the hdr field returned undefined when
PRODUCER_HDR_TRANSFER wasn't "hlg" or "pq", but the field is typed
as { transfer: HdrTransfer } | false. Returning false matches the
type and avoids a downstream undefined check.
Tests
- chunkEncoder.test.ts: replaced the previous "HDR options ignored"
assertions with 8 new specs covering BT.2020 + transfer tagging,
master-display/max-cll embedding, libx264 fallback behavior, GPU
H.265 + HDR (tags but no x265-params), and range conversion for
both SDR and HDR CPU paths.
- All 313 engine unit tests pass (5 new HDR specs).
Follow-ups (separate PRs):
- Producer regression suite runs in CI; not exercising HDR-tagged
chunkEncoder yet because no live caller sets options.hdr there.
…d-transfer caller error PR #370 review feedback (jrusso1020): - chunkEncoder: when codec=h264 and hdr is set, log a warning and strip hdr instead of emitting a half-HDR file (BT.2020 container tags + BT.709 VUI inside the bitstream). libx264 has no HDR support; the only honest output is SDR/BT.709. Caller is told to use codec=h265. - videoFrameExtractor: comment at the convertSdrToHdr call site clarifying that dominantTransfer is majority-wins; mixing PQ and HLG sources in a single composition is caller-error and the minority transfer's videos will be converted with the wrong curve. Render two compositions if you need both transfers. - docs/guides/hdr.mdx: limitations section now documents (a) H.264 + HDR is rejected at the encoder layer, and (b) GPU H.265 (nvenc, videotoolbox, qsv, vaapi) emits BT.2020 + transfer tags but does NOT embed master-display or max-cll SEI, since ffmpeg won't pass x265-params through hardware encoders. Acceptable for previews, not for HDR10 delivery.
Address jrusso1020's review on PR #371: add a path.win32 test suite that exercises isPathInside on Linux/macOS CI to catch accidental Unix-only assumptions (e.g. only splitting on "/") that would silently regress for Windows users. - isPathInside now accepts an optional `pathModule` (defaults to node:path) so tests can inject path.win32 / path.posix without changing call sites. - New describe block covers equality, direct/deep children, sibling-prefix rejection, traversal escapes, trailing-backslash normalization, and cross-drive rejection.
0947ad8 to
b7b30c6
Compare
7b268a9 to
5256a93
Compare
StreamingEncoder.close() and closeCaptureSession() are both invoked from renderOrchestrator's HDR cleanup path with a tracked-flag pattern, but the outer finally block may still re-call them if the inner cleanup raised before the flag flipped. Both must therefore be idempotent. streamingEncoder.close: add an inline comment explaining why each step (clearTimeout, removeEventListener, stdin.end gated on !destroyed, shared exitPromise) is safe to repeat. closeCaptureSession: previously NOT truly idempotent under browser-pool semantics — releaseBrowser decrements pooledBrowserRefCount, so calling it twice for the same acquire could close a browser another session still holds. Add per-session pageReleased/browserReleased flags to the CaptureSession interface and gate page.close()/releaseBrowser() behind them. Set the flag AFTER the await so a mid-cleanup throw still allows the outer defensive call to retry the unreleased resource.
b7b30c6 to
504e005
Compare
Address jrusso1020's review on PR #371: add a path.win32 test suite that exercises isPathInside on Linux/macOS CI to catch accidental Unix-only assumptions (e.g. only splitting on "/") that would silently regress for Windows users. - isPathInside now accepts an optional `pathModule` (defaults to node:path) so tests can inject path.win32 / path.posix without changing call sites. - New describe block covers equality, direct/deep children, sibling-prefix rejection, traversal escapes, trailing-backslash normalization, and cross-drive rejection.
Merge activity
|
Address jrusso1020's review on PR #371: add a path.win32 test suite that exercises isPathInside on Linux/macOS CI to catch accidental Unix-only assumptions (e.g. only splitting on "/") that would silently regress for Windows users. - isPathInside now accepts an optional `pathModule` (defaults to node:path) so tests can inject path.win32 / path.posix without changing call sites. - New describe block covers equality, direct/deep children, sibling-prefix rejection, traversal escapes, trailing-backslash normalization, and cross-drive rejection.
Address jrusso1020's review on PR #371: add a path.win32 test suite that exercises isPathInside on Linux/macOS CI to catch accidental Unix-only assumptions (e.g. only splitting on "/") that would silently regress for Windows users. - isPathInside now accepts an optional `pathModule` (defaults to node:path) so tests can inject path.win32 / path.posix without changing call sites. - New describe block covers equality, direct/deep children, sibling-prefix rejection, traversal escapes, trailing-backslash normalization, and cross-drive rejection.
Address jrusso1020's review on PR #371: add a path.win32 test suite that exercises isPathInside on Linux/macOS CI to catch accidental Unix-only assumptions (e.g. only splitting on "/") that would silently regress for Windows users. - isPathInside now accepts an optional `pathModule` (defaults to node:path) so tests can inject path.win32 / path.posix without changing call sites. - New describe block covers equality, direct/deep children, sibling-prefix rejection, traversal escapes, trailing-backslash normalization, and cross-drive rejection.
Address jrusso1020's review on PR #371: add a path.win32 test suite that exercises isPathInside on Linux/macOS CI to catch accidental Unix-only assumptions (e.g. only splitting on "/") that would silently regress for Windows users. - isPathInside now accepts an optional `pathModule` (defaults to node:path) so tests can inject path.win32 / path.posix without changing call sites. - New describe block covers equality, direct/deep children, sibling-prefix rejection, traversal escapes, trailing-backslash normalization, and cross-drive rejection.
Address jrusso1020's review on PR #371: add a path.win32 test suite that exercises isPathInside on Linux/macOS CI to catch accidental Unix-only assumptions (e.g. only splitting on "/") that would silently regress for Windows users. - isPathInside now accepts an optional `pathModule` (defaults to node:path) so tests can inject path.win32 / path.posix without changing call sites. - New describe block covers equality, direct/deep children, sibling-prefix rejection, traversal escapes, trailing-backslash normalization, and cross-drive rejection.

Summary
Five resource-management fixes in
renderOrchestrator.tsandfileServer.ts: HDR encoder cleanup on non-abort errors,frameDirMaxIndexCacheeviction, mid-transition abort responsiveness, pre-allocated transition buffers, and a path-traversal guard for the local file server.Why
Chunk 5ofplans/hdr-followups.md. These are independent leaks/hangs/security issues that had each been called out in prior PR reviews and never landed.What changed
5A — HDR encoder +
domSessioncleanup. The HDR streaming encoder anddomSessionwere spawned outside any outertry/finally, so a non-abort error between encoder spawn and the inner cleanup leaked the FFmpeg process and held the browser page open. Wrapped the entire HDR (and SDR streaming) capture path in atry/finallywith explicit*Closedflags, and defensively close both in the outerfinallyif they haven't been closed already.StreamingEncoder.close()andcloseCaptureSession()are both idempotent, so double-close is safe.5B —
frameDirMaxIndexCache+hdrFrameDirseviction.frameDirMaxIndexCacheis module-scoped and grew monotonically: every render added entries that were never removed. LiftedhdrFrameDirsto the outer scope, drop the matching cache entry in the per-videormSyncblock, and sweep any survivors in the outerfinally. The on-disk frames themselves were already torn down withworkDir; this just stops the in-process Map from leaking entries across renders.5C — Abort signal between scene A and scene B. During a shader transition the orchestrator captures scene A and scene B back-to-back inside a single outer frame iteration. An abort that arrived while scene A was capturing wouldn't be noticed until the next outer frame — after scene B had already been fully composited and discarded. Added
assertNotAborted()at the top of the inner[transBufferA, transBufferB]loop so abort is observed before the second scene's DOM seek + screenshot.5D — Pre-allocated transition buffers (already addressed). The transition buffers (
transBufferA,transBufferB,transOutput,normalCanvas) are pre-allocated outside the per-frame loop. The remainingBuffer.fromcopies sit in HDR transfer conversion (Chunk 8B territory) and image preload, neither of which is the per-frame hot path.5E —
fileServerpath-traversal guard.fileServer.tsjoinedcompiledDir/projectDirwith the request path and only checkedexistsSync+isFile.path.joinnormalizes..segments, soGET /../etc/passwdwould resolve to/etc/passwdand be served straight off disk if the file existed. Added anisPathInside(child, parent)helper that resolves both sides and compares prefixes with the platform separator appended (so/foodoesn't match/foobar), and rejects any candidate that lands outside its intended root.Test plan
bun run --filter @hyperframes/producer typecheckpasses.fileServer.test.ts13/13 pass (4 existing + 9 newisPathInsidecases covering same-path, nested, prefix-only siblings, escaping traversal, traversal that resolves back inside, trailing-slash handling, and relative-path resolution).ffmpegprocesses (5A).GET /../../../etc/passwdagainst the local file server returns 403/404 (5E).Stack
Chunk 5 of
plans/hdr-followups.md.