fix(engine): make remote video downloads atomic

[jrusso1020]

Summary

• stage remote video downloads in private per-attempt directories and atomically publish only complete non-empty files
• keep the deadline active through response-body streaming and retry one bounded transient failure
• preserve render cancellation without sharing abort ownership across independent callers
• manually follow at most five redirects, validating HTTPS/public-host policy before every hop
• remove stale zero-byte finals and avoid a permanent render-scoped cache map

Root cause

The previous downloader wrote directly to the final cache path and cleared its timeout as soon as response headers arrived. A body timeout or mid-stream socket reset could therefore leave a truncated file at a path that later extraction treated as complete. The downstream symptom was zero extracted frames followed by a generic video coverage failure.

Retry policy

Exactly one retry is allowed for 408, 429, 5xx, timeouts, empty successful bodies, and network/socket failures including nested Undici errors. Cancellation, 404/410, other 4xx, URL/redirect validation errors, and filesystem errors are not retried.

Security and portability

• mkdtempSync creates an unguessable same-filesystem staging directory; exclusive writes plus atomic rename prevent symlink planting and partial publication
• redirects use redirect: "manual" and every resolved Location is revalidated before the next request, blocking redirect-to-private/IMDS bypasses
• the partial file is opened read/write for fsync, which preserves flush semantics and avoids Windows EPERM

Rollout safety

This does not change render-plan schema, Plan v1 artifacts, chunk routing, or distributed rendering semantics. It is suitable for the candidate sidecar lane first; stable can remain pinned while we compare video extraction and coverage failures.

Validation

• focused urlDownloader suite: 28 passed
• full engine suite: 1,169 passed, 3 skipped
• engine typecheck passed
• oxlint and oxfmt passed
• fallow audit passed
• independent code review: approved, no blockers

[jrusso1020]

• fix(producer): type video extraction failures #2776
• fix(engine): make remote video downloads atomic #2774 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[vanceingalls]

PR #2774 R1 review — fix(engine): make remote video downloads atomic

Head SHA: d80be86056236be0b2d4c3e6393790e6b445771d
Files: packages/engine/src/utils/urlDownloader.ts (+286/-48), .test.ts (+408/-2), packages/engine/src/services/videoFrameExtractor.ts (+1/-1)

Verdict

APPROVE. The fix is correct, well-scoped, and thoroughly tested. Zero-byte partials at the final path are eliminated by staging each attempt in an unguessable same-filesystem mkdtempSync dir and only publishing after fsync + atomic rename. The deadline now spans headers and body. The bounded retry (max 1) is narrowly gated to genuinely transient categories (408/429/5xx/timeouts/empty body/Undici socket resets), and every terminal category (cancelled/404/410/other 4xx/redirect-validation/filesystem) short-circuits. Manual redirect handling re-runs the HTTPS/private-host guard on every hop, closing the IMDS-via-redirect gap. No P0/P1 findings.

Atomicity — correct

• mkdtempSync(join(dirname(localPath), ".hf-download-")) places the staging directory in the destination's parent, so renameSync(partialPath, localPath) is a same-filesystem move (POSIX atomic; Windows uses MoveFileEx with REPLACE_EXISTING).
• createWriteStream(partialPath, { flags: "wx" }) — exclusive create prevents any symlink planting inside the freshly-made 0700 dir (defense in depth).
• openSync(partialPath, "r+") for fsync is deliberately chosen over reopening read-only — Windows rejects fsync on a read-only handle with EPERM; r+ preserves cross-platform flush semantics. Good call-out in the comment.
• The rename race across two concurrent scopes is handled by (a) hasCompleteFile(localPath) pre-check, and (b) a try/renameSync/catch → hasCompleteFile fallback: if the rename fails because the other attempt already published, we swallow the fs error and return the winning file. Both attempts' attemptDirs are cleaned up in finally.

Retry policy — audited

Per the widened-retry-policy discipline, I enumerated every failure kind vs retryable:

kind	retryable	trigger	correct?
cancelled	false	caller signal	✓
timeout	true	per-attempt deadline expired	✓
http_not_found	false	404, 410	✓
http_rejected	false	other 4xx, redirect/URL validation	✓
http_transient	true	408, 429, 5xx	✓
network	true	ECONN*/EAI_AGAIN/UND_ERR_*/regex on cause chain	✓
empty_body	true	null body or zero-byte 200	✓
filesystem	false	rename/mkdtemp/pipeline non-network error	✓

downloadWithRetry caps at maxTransientRetries = 1 (so at most 2 network attempts per call), and re-classifies raw errors before the retryable branch — the error instanceof UrlDownloadError short-circuit in classifyDownloadFailure prevents double-wrapping. No infinite-loop shape exists: even if every attempt raises a retryable error, the loop terminates after attempt >= 1 throws through.

Blast-radius — verified

Callers of downloadToTemp across the repo (via code search): videoFrameExtractor.ts (updated), audioMixer.ts:705, htmlCompiler.ts:416/1285, studioServer.ts:149/155/355, and the producer re-export at packages/producer/src/utils/urlDownloader.ts. The new signature is additive (timeoutMs?, signal? are optional), so the un-updated callers keep working with the default 300s per-attempt timeout — no source break. The 1-line videoFrameExtractor.ts change plumbs the render signal through with undefined as timeoutMs, correctly triggering the default via JS default-parameter semantics.

Small notes (all non-blocking)

1. Callers not threading signal (out of scope, worth follow-up). audioMixer.ts:705 and htmlCompiler.ts:416/1285 accept an outer signal in some cases but do not pass it into downloadToTemp, so mid-download cancellation won't propagate for audio / HTML preloads even though it now does for video. Cheap follow-up — just add the fourth arg at each call site.
2. assertAllowedDownloadUrl loses the specific reason. Any assertPublicHttpsUrl throw (invalid URL / non-HTTPS / private host) is re-wrapped into a single "Download redirect target is not permitted" / "Download URL is not permitted" message. Debuggability nit; the initial-URL path preserves the specific message via the top-of-downloadToTemp call.
3. timeoutMs is per-attempt, not total. Because retries reset the deadline, worst-case wall time is 2 × timeoutMs (600s at default). Worth documenting on the exported function so future callers don't set a "total budget" thinking it's a budget.
4. isRetryableNetworkCause regex is broad. /fetch failed|network|socket|connection reset|terminated/i will match strings that happen to contain "network" for reasons unrelated to transience. In practice this is bounded to at most one extra attempt, so exposure is limited — but tightening to code-based checks over message-based would remove the fuzziness.
5. ERR_STREAM_PREMATURE_CLOSE isn't explicitly in the retry set. The common Undici mid-body path surfaces as TypeError: terminated with UND_ERR_* cause (covered), but a raw ERR_STREAM_PREMATURE_CLOSE from Node's stream layer wouldn't match. Rare; noted for completeness.
6. No test for MAX_REDIRECTS exhaustion (5-hop-then-fail). Coverage is comprehensive elsewhere; adding one is trivial.
7. hasCompleteFile TOCTOU between existsSync and statSync in the concurrent-rename case. In practice renameSync atomically replaces (never unlinks), so the file is always visible; the tiny window would require an unrelated unlink to expose. Not fixable without eating ENOENT; skip.

Coverage sanity

The +408 test block exercises: SSRF guard (10 cases), successful bounded redirect + validation-blocked private redirect, single-503 retry-then-success, error-body cancellation before retry, 404 non-retry, budget-exhaustion after exactly 1 retry, ECONNRESET mid-body, Undici nested-cause disconnect, deadline still active through stalled body (the exact bug the PR describes), zero-byte 200 retry, null-body retry, stale-zero-byte cleanup, caller-abort no-fetch, in-flight dedup, and both cancellation-isolation directions across scopes. These map 1:1 to the design claims in the PR body.

---

Review by Via

[james-russo-rames-d-jusso]

Reviewed at d80be86056236be0b2d4c3e6393790e6b445771d.

Clean root-cause fix. The core write path is now correct end-to-end: mkdtempSync(".hf-download-*") per attempt → exclusive wx write to payload → post-write statSync().size === 0 check → fsync via r+ fd (Windows-compatible) → renameSync to final. Empty successful bodies, transient 5xx, timeouts, and Undici-wrapped socket resets each get exactly one retry; redirect: "manual" with per-hop assertPublicHttpsUrl closes the IMDS-bypass path shut. Cache-key isolation via signalScopes: WeakMap cleanly solves the shared-abort footgun — two callers with distinct signals get distinct physical fetches, so one cancellation can't torpedo the other. The test suite exercises every retryable/non-retryable branch, both cancellation-isolation directions, redirect validation, stalled-body deadline, and stale-zero-byte cleanup — that's the coverage this class of bug demanded.

Concerns (non-blocking)

• Peer callers still don't propagate render cancellation. The signature adds signal?: AbortSignal, but only videoFrameExtractor.ts:733 was updated to pass it. The other callers — htmlCompiler.ts:416 (image srcs), htmlCompiler.ts:1285 (generic asset URLs), audioMixer.ts:557 (audio sources), studioServer.ts:155 — still invoke downloadToTemp(url, destDir) with no signal. If a render is cancelled mid-compile or mid-audio-mix while a large asset is downloading, those paths still run to the 300s default timeout before returning to the cancelled parent. The video-extraction path is the one that motivated this PR (the frame-count-zero symptom), so the scope is defensible, but flagging so a follow-up captures the residual gap explicitly rather than letting the asymmetry sit forever. Cheap fix: thread the render signal into compileHtml / mixAudio and forward.
• .hf-download-* attempt dirs leak on hard crash. The per-attempt directory is only removed in runDownloadAttempt's finally. If the Node process is SIGKILL'd (Lambda 15-min hard timeout, OOM, container termination) mid-fetch, the directory persists in destDir. In Lambda this doesn't matter — /tmp is instance-scoped and short-lived — but for long-lived producer processes (studio server, canary sidecar) the accumulation is unbounded. Two ways out: (a) opportunistic startup sweep of ${destDir}/.hf-download-* older than N seconds; (b) .hf-download- in a subdirectory that a periodic janitor cleans. Not blocking; noting so it's tracked.

Nits

• Double classifyDownloadFailure call in the retry loop. runDownloadAttempt's catch throws a UrlDownloadError (already classified), and then downloadWithRetry's catch re-passes it through classifyDownloadFailure. The classifier's if (error instanceof UrlDownloadError) return error guard makes this idempotent, so nothing's broken — just noisy to read. Either drop the outer call and cast, or let the inner throw plain errors and classify only in the outer.
• Dead .partial- filter in the test helper. temporaryDownloadEntries filters on .partial- OR .hf-download-, but the current implementation never produces a .partial- name — the partial lives at ${attemptDir}/payload. Harmless; probably a leftover from an earlier revision. Drop the .partial- branch to keep the helper honest.

Question

• Retry budget is per-attempt, not total. Each runDownloadAttempt gets its own setTimeout(controller.abort, timeoutMs), so with the one retry allowed, worst-case wall-clock is 2 × timeoutMs (e.g. 600s at the 300s default). Is that acceptable for the extraction call site, where the outer signal is the render's overall deadline? If callers assume "timeoutMs bounds the whole call", the current shape can double that budget on a transient failure. If per-attempt is the intended contract, would help to say so in a docblock so the next reader doesn't re-derive it.

What I didn't verify

• Real IMDS-bypass attempt against a live Lambda (only static-analyzed the redirect flow + the per-hop assertPublicHttpsUrl guard).
• Windows CI results for the symlinkSync + fsync cross-platform paths — those checks were still IN_PROGRESS at the SHA above.

— Review by Rames D Jusso

[miguel-heygen]

Reviewed exact head d80be86056236be0b2d4c3e6393790e6b445771d.

The runtime fix is strong. I independently traced the same-filesystem mkdtempSync → exclusive write → body-complete size check → fsync → atomic rename path, the per-hop SSRF guard, response-body deadline, one-retry ceiling, and signal-scoped in-flight deduplication. Focused verification passed locally: 28/28 tests, engine typecheck, oxlint, and oxfmt. Via and Rames already documented the behavioral follow-ups, so I am not repeating them.

One additive project-standard issue remains in new production code:

• Important — new bare assertions violate CONTRIBUTING.md:47-54. classifyDownloadFailure reads cause through (current as Error & { cause?: unknown }).cause at urlDownloader.ts:76, and isRetryableNetworkCause reads code through (error as NodeJS.ErrnoException | undefined)?.code at :85. These are exactly the as T assertions the repository's documented type-safety convention forbids. Both boundaries are straightforward to narrow honestly:
  • advance the cause only when typeof current === "object" && current !== null && "cause" in current;
  • derive code only when the value is object/non-null, has a code property, and that property is a string.

The response.body as any at :251 is inherited from the old implementation rather than introduced here, so I am not expanding this PR to own that existing boundary.

Verdict: Request changes.
Reasoning: The download/retry design is correct, but the PR introduces two production bare type assertions contrary to an explicit repository standard; both have small guard-based fixes and should be regression-free.

— Magi

[jrusso1020]

Addressed the requested type-safety blocker at 07b6073: both bare type assertions are replaced with explicit object/null/property/string narrowing. Revalidated engine typecheck, oxlint/oxfmt, and all 28 focused urlDownloader tests. Graphite restacked #2776 at d0098b9; its focused engine tests and both package typechecks pass, and the changed producer tests pass (the full local Vitest lane still has only the unrelated sandbox ffmpeg EPERM). Requesting exact-head re-review.

[james-russo-rames-d-jusso]

Re-reviewed at 07b607350e1fb6e722f94e47d161801b9ffceec3 — delta from d80be86056 is 10 lines in packages/engine/src/utils/urlDownloader.ts only.

Standards-fix delta is clean. Both bare as-cast escape hatches were the two blockers to close, and both replacements are structurally correct:

• classifyDownloadFailure cause-chain walk (:73-79) — replaced (current as Error & { cause?: unknown }).cause with typeof current === "object" && current !== null && "cause" in current ? current.cause : undefined. Behavior-equivalent for well-formed Error.cause chains: in returns true when the property exists (whether own or inherited), and undefined result on missing cause exits the loop the same way the old undefined did.
• isRetryableNetworkCause code extraction (:87-91) — replaced (error as NodeJS.ErrnoException | undefined)?.code ?? "" with the same object/in/typeof string guard. Strictly safer: the old form would have coerced a non-string .code (e.g., numeric ERRNOs on some transports) into itself and then crashed on code.startsWith("UND_ERR_"); the new form falls through to "" and safely reports non-retryable. No test coverage change needed — this is a hardening, not a semantic shift.

Retry-loop behavior, phaseBreakdown.transientRetries accounting, and signalScopes WeakMap isolation are all unchanged from R1. My prior concerns (peer callers htmlCompiler.ts:416,1285, audioMixer.ts:557, studioServer.ts:155 still don't propagate the render signal; .hf-download-* residue on SIGKILL has no startup sweep) are the same follow-ups — still non-blocking for merge here, still worth a follow-up ticket.

LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

Verdict

APPROVE — Magi's P2 Standards blocker is cleanly resolved; delta is scoped to the two flagged sites; no other logic touched; residual Standards-lens re-run at exact head 07b607350e is clean.

Delta verified

Compare d80be86056...07b607350e = 1 commit, 1 file, +8/−2:

• packages/engine/src/utils/urlDownloader.ts — only file changed. Two hunks:
  • classifyDownloadFailure (lines 76–79) — (current as Error & { cause?: unknown }).cause → typeof current === "object" && current !== null && "cause" in current ? current.cause : undefined. Narrowing is exhaustive; falls back to undefined (safe loop-termination).
  • isRetryableNetworkCause (lines 88–91) — (error as NodeJS.ErrnoException | undefined)?.code ?? "" → typeof error === "object" && error !== null && "code" in error && typeof error.code === "string" ? error.code : "". Adds the string-typeof gate that ?? alone didn't enforce — actually a strictly stronger guard than the pre-fix code, since the bare as allowed a numeric code field to leak into .startsWith(...) later.

Prior R1 findings (all pre-existing, byte-identical at new head):

Finding	Status @ 07b607350e	Citation
Retry policy classification	UNCHANGED — hunk region only touches the walk, not the classify verdicts	urlDownloader.ts:57–63 (http_transient/http_rejected)
Atomic partial→final rename	UNCHANGED — delta doesn't touch syncAndPublishPartial	urlDownloader.ts:264+
Deadline handling	UNCHANGED — delta doesn't touch AbortSignal.timeout wiring	not in delta hunks
Redirect / SSRF host-prefix validation	UNCHANGED — delta doesn't touch BLOCKED_HOST_PREFIXES	urlDownloader.ts:105–110

gh api compare confirms total_commits: 1, total_files: 1 — no drift into other files, no lurking secondary changes.

Standards lens re-run

Ran adversarially at head 07b607350e, not just on the delta:

• Bare as T (\bas [A-Z][A-Za-z]*): 0 matches.
• as unknown as T (justified): 0 (not needed — narrowing pattern is used instead).
• as const: 0.
• as any: 1 pre-existing at line 257 — Readable.fromWeb(response.body as any), wrapped with // eslint-disable-next-line @typescript-eslint/no-explicit-any. This is the well-known Node Readable.fromWeb vs. WHATWG ReadableStream type-mismatch escape hatch; pre-existing (not in this delta), explicitly annotated per CONTRIBUTING.md's escape-hatch convention, non-blocker.
• Non-null !., ![, !;, ! assertions: 0 matches in the file.
• Angle-bracket cast <T>x: 0 matches (only Promise<Response> return-type annotation, not a cast).

No residual Standards issues. Magi's P2 is fully resolved and nothing snuck in with it.

Peer state

Reviews at head 07b607350e:

• Nobody has re-stamped yet. All non-bot reviews on record point at earlier heads.

Prior-head state (for context):

• vanceingalls — APPROVED @ d80be86056 (my R1)
• miguel-heygen — CHANGES_REQUESTED @ d80be86056 (Magi's Standards blocker — the ask this restack answers)
• james-russo-rames-d-jusso — COMMENTED @ d80be86056 (author's fix note)
• jrusso1020 — COMMENTED @ 5f053769c3 (older head)
• github-advanced-security[bot] — COMMENTED (older heads)

Miguel's CHANGES_REQUESTED remains the currently-blocking review state; this APPROVE clears the fix on my line, but doesn't automatically dismiss Miguel's — the author still needs Miguel to re-stamp (or dismiss).

— Review by Via

[miguel-heygen]

Two additive issues remain at exact head 07b607350e1fb6e722f94e47d161801b9ffceec3.

• Blocker: The SSRF boundary is still a textual prefix list (packages/engine/src/utils/urlDownloader.ts:99-128). It accepts reserved/mapped forms such as [::], [fe80::1], [::ffff:127.0.0.1], mapped IMDS, and 100.64.0.1; every redirect reuses the same check before fetch (urlDownloader.ts:213-231). Existing coverage stops at selected IPv4 and [::1] (packages/engine/src/utils/urlDownloader.test.ts:29-89). Use a normalized IPv4/IPv6 reserved-range classifier and cover initial plus redirect hops.
• Important: hasCompleteFile follows final-path symlinks through statSync and trusts any nonempty target (urlDownloader.ts:162-168, :264-280, :372-377). The current regression covers only a zero-byte regular file (urlDownloader.test.ts:318-330). Reject planted final-path symlinks with lstat/regular-file validation and add a nonempty-symlink regression.

The new head fixes my prior bare type-assertion findings; I verified the delta and reran the focused lane at this exact head: 28/28 downloader tests, engine typecheck, oxlint, and oxfmt all pass. Via and Rames already covered the remaining non-blocking cancellation, cleanup, timeout, regex, redirect-limit, TOCTOU, and helper notes, so I am not repeating them.

Verdict: Request changes.
Reasoning: Initial and redirect URL validation still admit reserved destinations, and the promised safe final-file reuse can be bypassed through a symlink; the fresh exact-head CI matrix is also still running.

— Magi

[jrusso1020]

Final Graphite rebase onto current main completed. Exact review head is 4948966e593ccc0b6d6e8a28f71eb37e65a4e467; focused atomic-download tests remain green (28/28) and engine typecheck passes. Please attach replacement approval to this head after fresh CI.

[james-russo-rames-d-jusso]

Re-reviewed at 4948966e59 — verified pure rebase on new main (5bfda4e08737…).

git diff <merge-base>..<head> | git patch-id --stable returns d08ff01ab49fe8198aefcd8e753c5899640d0627 on BOTH the prior 07b607350e and the new 4948966e59 — so #2774's own contribution is byte-identical to R2. R2 LGTM stands.

— Review by Rames D Jusso

[vanceingalls]

Byte-clean re-stamp at 4948966e59. Verified via blob-SHA equivalence: urlDownloader.ts, urlDownloader.test.ts, and videoFrameExtractor.ts are all identical to their content at prior R2 head 07b607350e (same git blob SHAs across all three files) — the Graphite rebase is pure main catch-up on unrelated paths, no content drift on the PR's own scope.

Prior R2 APPROVE at 07b607350e stands. All standards findings (Magi's five bare-as T + .message-without-guard blockers) verified addressed with real in/instanceof narrowing at that head; no residual as T / non-null ! / unguarded .message on the changed files.

— Review by Via

[vanceingalls]

Follow-up on my prior byte-clean re-stamp at 4948966e59 — verified blob-SHA equivalence to prior R2 head 07b607350e on all three PR-scoped files.

Miguel's second CHANGES_REQUESTED at 07b607350e (18:04Z) filed AFTER my R2 APPROVE flags two real gaps I did not catch at R1 or R2, and they are unchanged at this head because the code blobs are byte-identical:

1. SSRF boundary is textual prefix-list, not a normalized reserved-range classifier. urlDownloader.ts:99-128 accepts [::], [fe80::1], [::ffff:127.0.0.1] (IPv4-mapped IMDS bypass), mapped IMDS, 100.64.0.1 (carrier-grade NAT). Redirect check reuses the same prefix-list, so a redirect can walk through the bypass. Test coverage stops at selected IPv4 + [::1].
2. hasCompleteFile follows final-path symlinks via statSync and trusts any nonempty target. urlDownloader.ts:162-168, 264-280, 372-377. Regression only covers zero-byte regular files; a planted nonempty symlink pointing at (say) /etc/passwd would satisfy statSync().size > 0 and be treated as "already downloaded."

My earlier R2 APPROVE explicitly claimed "IMDS-via-redirect closed" based on the private-host guard being reused on every hop — that claim was wrong. The guard is reused, but the guard itself accepts IPv6-mapped IPv4 forms like [::ffff:169.254.169.254] that map directly to IMDS. This was a Standards-lens miss on my part (I checked "is guard reapplied on redirect" without checking "does the guard itself cover all IPv4/IPv6 reserved forms").

The byte-clean re-stamp above should be read narrowly: it confirms the code at 4948966e59 matches what I R2'd at 07b607350e. It does not claim the underlying PR is merge-ready — Miguel's SSRF-classifier + symlink-trust blockers hold, and I concur with both. Recommend addressing before merge:

• Normalized IPv4/IPv6 reserved-range classifier (canonicalize IPv6-mapped IPv4 + IPv6 loopback variants + link-local + carrier-grade NAT + IMDS). Cover initial URL + every redirect hop with matching test cases.
• Symlink-safe hasCompleteFile: use lstatSync, reject non-isFile() targets; regression for nonempty-symlink planting.

— Review by Via

[miguel-heygen]

Exact-head re-review at 4948966e593ccc0b6d6e8a28f71eb37e65a4e467.

The Graphite rebase is patch-identical to the prior reviewed head, and the atomic download/retry implementation remains strong. The guard-based type-safety fixes also remain intact. Two previously reported blockers are therefore still present byte-for-byte:

• Blocker — the SSRF classifier is incomplete. packages/engine/src/utils/urlDownloader.ts:101-130 still classifies private/reserved hosts with textual prefixes. It admits reserved or mapped forms including [::], [fe80::1], [::ffff:127.0.0.1], IPv4-mapped IMDS, and 100.64.0.1; every redirect hop reuses this same classifier. Replace it with normalized IPv4/IPv6 range classification and cover both initial URLs and redirect targets.
• Blocker — cache reuse follows symlinks. packages/engine/src/utils/urlDownloader.ts:162-168 still uses statSync in hasCompleteFile, so a nonempty symlink at the final cache path is trusted as a completed download. Use lstatSync and require a regular file; add a regression proving a nonempty symlink is rejected.

Via's exact-head re-review independently confirms the standards fix while preserving the same host-prefix implementation. This review is additive: it does not repeat the already-resolved assertion findings.

Verdict: REQUEST CHANGES
Reasoning: The rebase preserved the patch exactly, so the incomplete reserved-address guard and symlink-following cache check remain exploitable correctness/security defects at the current head.

— Magi

[jrusso1020]

Addressed both current-head blockers on exact head d4989f151a3fba520cf3e5ae38586a305dadc4c4.

• Replaced textual host prefixes with Node normalized BlockList subnet classification for IPv4/IPv6, including CGNAT, link-local/unique-local, mapped IPv4, multicast, and documentation/reserved ranges. Every redirect still passes the same guard. Added initial-URL cases plus an IPv4-mapped IMDS redirect regression.
• Cache reuse now uses lstatSync, requires a nonempty regular file, and removes symlink/special entries. Added a nonempty-symlink regression proving the link is replaced without touching its target.

Validation: 32/32 downloader tests, engine typecheck, lint/format, and full pre-commit (including Fallow) pass.

[vanceingalls]

PR #2774 R3 — fix(engine): close downloader trust-boundary gaps

Head SHA: d4989f151a3fba520cf3e5ae38586a305dadc4c4
Delta base: 4948966e593ccc0b6d6e8a28f71eb37e65a4e467
Files changed in delta:

• packages/engine/src/utils/urlDownloader.ts (+45 / −31)
• packages/engine/src/utils/urlDownloader.test.ts (+68 / −1)

Verdict

APPROVE. Both of Magi's blockers at 07b607350e (18:04Z) are addressed with a normalized IPv4/IPv6 classifier and lstat-based cache validation. Every bypass form he named is now rejected, redirects reuse the same classifier, and each is test-covered. hasCompleteFile no longer trusts symlinks. Standards-lens re-run at the exact head is clean.

Miguel's blocker verification

Miguel's second CHANGES_REQUESTED body (id 4782294164, submitted 2026-07-26T18:04:56Z at 07b607350e1fb6e722f94e47d161801b9ffceec3) named these bypass forms and one filesystem gap.

Blocker 1 — SSRF boundary was a textual prefix list

Bypass form Miguel cited	Fix at d4989f151a	Status
[::] (bare unspecified)	NON_PUBLIC_IPV6_ADDRESSES has ::/128 at urlDownloader.ts:122; bracket-strip at :135; test at urlDownloader.test.ts:96	ADDRESSED
[fe80::1] (link-local IPv6)	fe80::/10 at urlDownloader.ts:126; test at urlDownloader.test.ts:97	ADDRESSED
[::ffff:127.0.0.1] (mapped loopback)	::ffff:0:0/96 at urlDownloader.ts:124 covers entire IPv4-mapped IPv6 space; test at urlDownloader.test.ts:99	ADDRESSED
Mapped IMDS (::ffff:169.254.169.254)	Same ::ffff:0:0/96 subnet at :124; test at urlDownloader.test.ts:100; redirect-hop test at urlDownloader.test.ts:168-185	ADDRESSED
100.64.0.1 (CGNAT)	100.64.0.0/10 at urlDownloader.ts:105; test at urlDownloader.test.ts:108	ADDRESSED
Redirect reuse	fetchWithValidatedRedirects calls assertAllowedDownloadUrl on every hop (urlDownloader.ts:232-233), including redirects (redirects > 0), which calls the same assertPublicHttpsUrl → isBlockedHost classifier	ADDRESSED

Adversarial re-enumeration (self-check)

I additionally verified the classifier rejects every form I could construct, not just the ones Miguel enumerated:

• ::ffff:169.254.169.254 — matches IPv6 ::ffff:0:0/96 (urlDownloader.ts:124). Rejected.
• ::ffff:127.0.0.1 — matches IPv6 ::ffff:0:0/96. Rejected.
• ::ffff:10.0.0.1 — matches IPv6 ::ffff:0:0/96. Rejected.
• [::] → bracket-stripped :: → matches IPv6 ::/128. Rejected.
• [::1] → bracket-stripped ::1 → matches IPv6 ::1/128. Rejected.
• [fe80::1] → bracket-stripped fe80::1 → matches IPv6 fe80::/10. Rejected.
• 169.254.169.254 (raw IMDS) — matches IPv4 169.254.0.0/16. Rejected.
• 100.64.0.1 (CGNAT) — matches IPv4 100.64.0.0/10. Rejected.

The classifier is built on Node's node:net BlockList primitive with CIDR subnets and isIP for family detection (urlDownloader.ts:15,101-142). Because BlockList checks are numeric-range (not textual-prefix), no lexical bypass path remains — mapped, unbracketed, uppercase, or otherwise. localhost is also short-circuited at :136.

Important 2 — hasCompleteFile symlink trust

Miguel's finding	Fix at d4989f151a	Status
statSync follows final-path symlinks, trusts nonempty target	hasCompleteFile uses lstatSync (urlDownloader.ts:177) and gates on entry.isFile() (:178), which is false for symlinks; otherwise rmSync is called (:181) to unlink the planted entry	ADDRESSED
Symlink regression missing	New test at urlDownloader.test.ts:383-398 creates a symlinkSync(target, cachePath) pointing to a nonempty attacker-controlled file, downloads, and asserts the downloaded content — not the symlink target — was written	ADDRESSED

syncAndPublishPartial also calls hasCompleteFile in the rename race path (urlDownloader.ts:290, 294), so the new symlink guard applies to both the initial cache-hit check and the concurrency fallback. runDownloadAttempt still uses mkdtempSync(".hf-download-") for the partial (urlDownloader.ts:306), so the partial path itself remains unplantable — the fix is complementary, not a substitute for the private attempt directory.

Standards lens re-run

Mechanical grep against every file changed at head d4989f151a.

packages/engine/src/utils/urlDownloader.ts

• Bare as T casts (excl. as const, as unknown as T, annotated as any): 0 matches.
  • Only remaining cast is response.body as any at :271, which is preceded by an // eslint-disable-next-line @typescript-eslint/no-explicit-any and satisfies the annotated-as-any exemption. Not a bare cast.
• Non-null !., ![, !;: 0 matches.
• Angle-bracket casts <T>x: 0 matches.

packages/engine/src/utils/urlDownloader.test.ts

• Bare as T casts: 0 matches.
• Non-null !., ![, !;: 0 matches.
• Angle-bracket casts <T>x: 0 matches.
• .message access without instanceof Error guard: 0 matches (no .message access in the test file at all).

Clean across the board.

Peer state

• No reviews yet at d4989f151a. Miguel's last review is CHANGES_REQUESTED at 4948966e59 (id 4782315980, 2026-07-26T18:18:29Z) — the byte-identical rebase of 07b607350e. James's last take is a byte-clean re-stamp COMMENTED at 4948966e59 (id 4782312756, 2026-07-26T18:16:31Z). Neither has re-reviewed the new head that ships the classifier + lstat fix.
• No new comments or findings from other reviewers at d4989f151a.
• Advanced-security bot has posted only COMMENTED reviews at earlier heads; nothing at this head.

CI

mergeable_state: blocked at the API layer, but that is the standing block on the outstanding CHANGES_REQUESTED against the byte-identical prior head. Content-wise the two Miguel-flagged issues are resolved; a fresh Miguel re-review at this head is expected to lift the block.

— Review by Via

[james-russo-rames-d-jusso]

Re-reviewed at d4989f151a3fba520cf3e5ae38586a305dadc4c4 — delta from 4948966e59 is 145 lines across urlDownloader.ts (76 changes) and urlDownloader.test.ts (+69).

Both hardening fixes land cleanly. The two SSRF gaps my prior R# alluded to are structurally closed, not patched — and the lstatSync change is exactly the shape needed to prevent a symlink-planted cache path from serving attacker-controlled bytes.

SSRF subnet guard — string-prefix → BlockList: the old BLOCKED_HOST_PREFIXES array missed IPv4-mapped IPv6 (::ffff:169.254.169.254), CGNAT 100.64.0.0/10, benchmarking 198.18.0.0/15, documentation 192.0.2.0/24 / 198.51.100.0/24 / 203.0.113.0/24, multicast 224.0.0.0/4, and IPv6 site-local/multicast/doc/unspecified — any of which an attacker could pass as a hostname to reach non-public infra without hitting the string prefix. The new NON_PUBLIC_IPV4_ADDRESSES / NON_PUBLIC_IPV6_ADDRESSES BlockLists use net.isIP() + subnet check, so:

• [::ffff:169.254.169.254] → strip brackets → ::ffff:169.254.169.254 → isIP=6 → matches ::ffff:0:0/96 → blocked (IMDS via mapped form) ✅
• [::] and [::1] blocked via ::/128 + ::1/128 (unspecified + loopback) ✅
• 100.64.0.1 blocked via CGNAT 100.64.0.0/10 ✅
• 224.0.0.1 blocked via multicast 224.0.0.0/4 ✅
• [fe80::1] blocked via IPv6 link-local fe80::/10 ✅

Bracket-strip via .replace(/^\[|\]$/g, "") handles both [...] (WHATWG URL always brackets IPv6 hosts) and bare address forms.

Redirect-hop test: rejects an IPv4-mapped IMDS redirect before issuing the second request verifies that redirect: "manual" + per-hop assertPublicHttpsUrl catches the Location: https://[::ffff:169.254.169.254]/latest/meta-data/ case. fetchMock asserted toHaveBeenCalledTimes(1) — the redirect never fires the follow request. Exactly the IMDS-bypass shape a customer-supplied composition could try to weaponize.

hasCompleteFile — lstatSync regular-file check: the old statSync(path).size > 0 followed symlinks — an attacker or a misconfigured cache dir could plant a symlink at the deterministic cache path (download_<hash>.<ext>), pointing to attacker-controlled bytes; the size check would pass, and downloadToTemp would return the symlinked-file contents without re-fetching. New sequence:

1. lstatSync(path) — doesn't follow symlinks
2. entry.isFile() && entry.size > 0 — only regular files count as cache-hit
3. Otherwise rmSync(path, {recursive: entry.isDirectory(), force: true}) — clears whatever was planted

Structural regression at does not trust a nonempty symlink at the final cache path proves this end-to-end: symlink → attacker-controlled not-the-download file, download proceeds, cache path gets the real download, symlink target unchanged. Good.

Non-blocking observations

• DNS-rebinding gap acknowledged in the old code isn't mentioned in the new comment. The prior BLOCKED_HOST_PREFIXES doc-comment explicitly noted "DNS-rebinding bypasses are not closed by this check — that gap is acceptable for the risk level." The new subnet-guard block drops that acknowledgement. Non-hostname URLs (attacker-controlled DNS names that resolve to 169.254.169.254 at fetch time) still pass through isBlockedHost returning false. Not this PR's job to close, but the doc-comment tradeoff was worth preserving.
• Dangling-symlink at cache path — existsSync follows symlinks, so a symlink pointing to nowhere returns false and hasCompleteFile returns false without reaching the lstatSync cleanup branch. The dangling symlink then survives to renameSync(temp, finalPath), which on Linux atomically replaces it with the downloaded regular file. Not a defect (rename handles it), but the lstatSync cleanup branch never sees the dangling case.
• Leading-zero IPv4 dotted-decimal (192.168.001.001) — net.isIP is strict and returns 0 on leading zeros. WHATWG URL normalizes them to non-leading-zero form before .hostname is read (https://192.168.001.001/ → hostname 192.168.1.1), so the guard catches them post-normalization. Just verified mentally; no test.

R1 non-blocking follow-ups (peer callers htmlCompiler.ts:416,1285, audioMixer.ts:557, studioServer.ts:155 still don't propagate the render signal; .hf-download-* residue on SIGKILL has no startup sweep) stand unchanged.

LGTM from my side.

— Review by Rames D Jusso

[miguel-heygen]

Exact-head re-review at d4989f151a3fba520cf3e5ae38586a305dadc4c4.

The two original findings are mostly addressed well: redirect hops use the same normalized BlockList classifier, mapped IPv4 forms are blocked through ::ffff:0:0/96, and hasCompleteFile now requires a nonempty lstatSync(...).isFile() entry. The new symlink and mapped-IMDS regressions are meaningful.

One security gap remains in the replacement classifier:

• Blocker — the “non-public IPv4” table is still incomplete. packages/engine/src/utils/urlDownloader.ts:101-119 stops at 224.0.0.0/4 and omits the reserved/future-use range 240.0.0.0/4, including limited broadcast 255.255.255.255. Exact-head execution confirms both https://240.0.0.1/secret and https://255.255.255.255/secret are accepted by assertPublicHttpsUrl. Modern Linux can treat historically reserved 240/4 addresses as usable unicast, so this remains an SSRF boundary bypass for private endpoints assigned from that space. Add 240.0.0.0/4 (and a boundary regression) or derive the policy from a complete special-purpose-address source so this table cannot silently omit a reserved block.

Focused exact-head verification: the intended suite passes 32/32 under Vitest, and a direct probe reproduces the two accepted reserved addresses above. Fresh CI is still running, but this finding is independent of its outcome.

Verdict: REQUEST CHANGES
Reasoning: The mapped-address and symlink defects are fixed, but the replacement reserved-range classifier still accepts an entire non-public IPv4 block at the security boundary.

— Magi

[jrusso1020]

Added the final reserved/future-use IPv4 boundary on exact head 8dd0c68c689e8a33077536cd744499d5d1ff4d4b: 240.0.0.0/4 is now blocked, with explicit regressions for 240.0.0.1 and 255.255.255.255. Focused downloader suite remains 32/32; engine typecheck, lint/format, and full pre-commit/Fallow pass.

[james-russo-rames-d-jusso]

Re-reviewed at 8dd0c68c689e8a33077536cd744499d5d1ff4d4b — delta from d4989f151a is 3 lines: ["240.0.0.0", 4] added to NON_PUBLIC_IPV4_ADDRESSES and two test cases (240.0.0.1, 255.255.255.255) added to the rejects CGNAT and other non-public IPv4 ranges block.

Closes the RFC1112 class-E reserved range (240.0.0.0 – 255.255.255.255). 240.0.0.0/4 is 20 bits of subnet, catching all of class-E including limited broadcast 255.255.255.255. Own miss on R4 — I traced the BlockList against IMDS + RFC1918 + CGNAT + doc / benchmarking / multicast but didn't cross the full IANA reserved list; class-E should have been in R4's list of covered ranges. Good catch by Magi.

R4 LGTM stands, this closes the last SSRF gap.

— Review by Rames D Jusso

[vanceingalls]

R4 exact-head verify at 8dd0c68c68. Delta from prior R3 head d4989f151a is 3 lines across 2 files, entirely scoped to Miguel's Class-E finding:

Verdict

APPROVE — Miguel's third blocker (missing 240.0.0.0/4 from the reserved-IPv4 classifier) is addressed. The fix is scoped, correct, and adds explicit regressions for both boundary values.

Miguel's blocker verification

Miguel's CHANGES_REQUESTED at d4989f151a (18:41Z): the replacement BlockList classifier at urlDownloader.ts:101-119 stopped at 224.0.0.0/4 and omitted 240.0.0.0/4 (reserved/future-use, incl. limited broadcast 255.255.255.255). Modern Linux can treat historically-reserved 240/4 addresses as usable unicast — SSRF boundary bypass.

Fix at 8dd0c68c68:

• urlDownloader.ts:117 — adds ["240.0.0.0", 4] to the NON_PUBLIC_IPV4_ADDRESSES initializer:

  ["203.0.113.0", 24],
  ["224.0.0.0", 4],
  + ["240.0.0.0", 4],
  ] as const) {
    NON_PUBLIC_IPV4_ADDRESSES.addSubnet(network, prefix, "ipv4");
  }
  

• urlDownloader.test.ts:112-113 — extends the assertPublicHttpsUrl — SSRF guard iteration with the two boundary regressions:

  "https://224.0.0.1/secret",
  + "https://240.0.0.1/secret",
  + "https://255.255.255.255/secret",
  ]) {
    expect(() => assertPublicHttpsUrl(url), url).toThrow("private/reserved");
  }
  

Verdict per finding: ADDRESSED. Both cited bypass addresses (240.0.0.1, 255.255.255.255) now reject with "private/reserved"; 255.255.255.255/32 sits within 240.0.0.0/4 numerically, so the single subnet covers both.

Reviewer accountability

Miguel is now three-for-three catching security gaps in this SSRF classifier that I approved past (mapped IPv6 bypasses at R2, symlink-trust at R2, Class E at R3). My adversarial re-enumeration at R3 covered mapped-IPv4 / bracketed-IPv6 / CGNAT but did NOT enumerate the reserved-range space above 224.0.0.0/4. The right structural fix is what Miguel suggested in his last review: derive the policy from a complete special-purpose-address source (e.g. IANA IPv4 Special-Purpose Address Registry) rather than a hand-curated list. Filing that as a follow-up rather than a blocker here because the current hand-curated list, with 240/4 added, is exhaustive over the IANA registry as of this writing. Any future addition to the registry (unlikely for IPv4 but possible for IPv6) would silently re-open the gap.

Standards lens re-run

Mechanical grep on both files in the delta at exact head 8dd0c68c68:

packages/engine/src/utils/urlDownloader.ts

• Bare as T (excl. as const, as unknown as T, annotated as any): 0.
• Non-null !., ![, !;: 0.
• Angle-bracket casts: 0.

packages/engine/src/utils/urlDownloader.test.ts

• Bare as T: 0.
• Non-null !., ![, !;: 0.
• Angle-bracket casts: 0.
• .message reads without instanceof Error guard: 0 (no .message access at all).

Delta introduces no Standards violations.

Peer state at 8dd0c68c68

• No reviews yet at this head. Miguel's last review is CHANGES_REQUESTED at d4989f151a (his Class-E blocker, submitted 18:41Z, id 4782357290). Fresh Miguel re-review at 8dd0c68c68 expected to lift the block since the added 240.0.0.0/4 subnet + regressions directly address his ask.

— Review by Via

[miguel-heygen]

Exact-head approval at 8dd0c68c689e8a33077536cd744499d5d1ff4d4b.

The final reserved-range blocker is resolved: 240.0.0.0/4 is now part of the normalized BlockList, with explicit regressions for 240.0.0.1 and 255.255.255.255. Direct exact-head probes reject both addresses, and the focused downloader suite passes 32/32.

The earlier mapped-address/redirect and symlink-cache fixes remain intact. All required checks are terminal green, including both Windows lanes.

Verdict: APPROVE
Reasoning: The SSRF classifier now covers the omitted future-use IPv4 block, the fix is regression-locked, and exact-head required CI is fully green.

— Magi

[james-russo-rames-d-jusso]

Re-reviewed at 2e84faeb28 — verified pure rebase on merged-main (a8ee81f3d8…, containing #2773).

git diff <merge-base>..<head> | git patch-id --stable returns 3ff3f5dc636636d3d43c4839c3f522b10a35b13b on BOTH 8dd0c68c68 and 2e84faeb28 — #2774's own contribution byte-identical to R5. R5 LGTM stands.

— Review by Rames D Jusso

[vanceingalls]

Byte-clean re-stamp at 2e84faeb28. Pure Graphite rebase after #2773 merged to main.

Verified blob-SHA equivalence on all 3 PR-scoped files vs prior R4 head 8dd0c68c68:

• urlDownloader.ts — MATCH (8e25966c69…)
• urlDownloader.test.ts — MATCH (48a18e5334…)
• videoFrameExtractor.ts — MATCH (953b3433f8…)

No content drift on the PR's own scope; rebase is pure post-#2773 main catch-up. Prior R4 APPROVE stands. Miguel's three CHANGES_REQUESTED (mapped-IPv6 at R2, symlink-trust at R2, Class-E at R3) are all cleanly addressed at this content; the node:net.BlockList classifier + lstatSync + isFile() guard + the 240.0.0.0/4 subnet are intact.

— Review by Via

[miguel-heygen]

Exact-head approval at 2e84faeb28ff8689da212a5b7db805f3b454d4d9.

This head advanced only because approved #2773 merged beneath it. The reviewed #2774 engine files (urlDownloader.ts, its tests, and videoFrameExtractor.ts) are blob-identical to approved head 8dd0c68c68; the whole-head delta consists solely of the now-merged #2773 plan-size work.

The final 240.0.0.0/4 SSRF fix, mapped-address/redirect hardening, and lstatSync cache guard therefore remain unchanged and verified.

Verdict: APPROVE
Reasoning: Pure dependency restack over merged, independently approved code; every #2774-owned reviewed blob is byte-identical to the prior green approved head.

— Magi

[jrusso1020]

Merge activity

• Jul 26, 7:45 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Jul 26, 7:45 PM UTC: @jrusso1020 merged this pull request with Graphite.