fix(engine): validate remote download integrity - #2938

R1 max-effort review — remote download integrity

Focused hardest-look pass on the three axes James called out: cross-process cache locking / publication races, caller-vs-server checksum retry classification, and external stylesheet redirect / timeout / signed-URL redaction. Also ran the general lens set (deferred parity, widened search for other download sites, presence-vs-semantics test discipline, and pre-existing invariants).

Overall: this is a large, well-designed hardening pass with meaningful tests that assert semantics (retryable flag, specific kind, byte counts, redaction patterns) rather than presence. All existing callers of downloadToTemp were migrated in one PR (gh api search/code confirms no orphan callers), the CLI capture path is orthogonal and out of scope, and the linkSync-based atomic no-clobber publication holds correctness even when the surrounding lock is contested. Every finding I have is low-severity — no P0, no blocker on the three axes.

Axis 1 — stale-cache locking / publication (mostly clean, one boundary nit)

• Directory-lock via mkdirSync(EEXIST) at ${localPath}.hf-lock; publication via linkSync (atomic no-clobber) then unlinkSync(partialPath); same-filesystem partial via mkdtempSync(dirname(localPath), ".hf-download-") — so linkSync cannot EXDEV. Good.
• 3-pass re-inspection loop plus sameCacheEntry (dev/ino/size/mtimeMs) guards against a mixed-version peer swapping the file underneath inspectExistingFile. Good.
• Concurrent caller-checksum scopes isolated via validationScope in both inFlightKey and getFilenameFromUrl(url, validationScope) — two callers with different expected SHAs get different physical files. The versioned.mp4 test locks this in.
• nit packages/engine/src/utils/urlDownloader.ts:267,317-322 (Axis 1, boundary lock invariant). CACHE_LOCK_STALE_MS = 5 * 60_000 is exactly equal to the default timeoutMs = 300000 in downloadToTemp. On a legitimate slow download that runs near the timeout budget, a second process's Date.now() - statSync(lockPath).mtimeMs > CACHE_LOCK_STALE_MS predicate can flip true right around when the holder finishes: peer B rmSyncs A's lock and mkdirSyncs its own; A's releaseLock() then rmSyncs B's live lock; the lock invariant "at most one publisher at a time" degrades. Correctness is saved by linkSync no-clobber + race_reused inspection, so this is wasted work rather than corruption — but the invariant is worth restoring. Suggested fix: pick CACHE_LOCK_STALE_MS >= 2 * max_expected_timeoutMs, or periodically utimesSync(lockPath) while the holder is streaming, or use flock/fcntl semantics that don't depend on wall-clock. Not blocking.

Axis 2 — caller-vs-server checksum classification (clean)

• Caller SHA path: checksumMismatchError("caller", …) returns retryable: false, locallyRetryable: true. Repeated caller mismatches consume the one clean in-call refetch and then surface as non-retryable upstream (caller-checksum.mp4 test).
• Server SHA path (x-amz-checksum-sha256 non-COMPOSITE, digest: sha-256=…, or content-md5): retryable: true, still locallyRetryable; repeated server mismatches escalate to retryable upstream. Composite S3 checksums explicitly ignored — no client-fabricated hash trap (per feedback_hand_typed_replica_is_not_a_repro, the caller checksum is trusted only because it originates from a distinct trust boundary, not the same fetch).
• content-encoding branch: expectedBytes is only asserted when encoding is identity, so gzipped bodies do not spuriously trip length_mismatch. The request also pins accept-encoding: identity so declared-length semantics are stable.
• normalizeCallerSha256 rejects malformed input synchronously — no fetch cost on obviously bad input.
• Range/206 handling: unsolicited or malformed 206 and Content-Range on 200 all classified as range_protocol, retryable: true with rangeDisposition telemetry — tests cover unsolicited, malformed, and content-range-on-200.

Axis 3 — redirect / timeout / redaction (clean)

• MAX_REDIRECTS = 5, per-hop assertPublicHttpsUrl runs before each fetch, redirect: "manual" prevents Undici auto-following into IMDS, and resolveRedirectUrl uses new URL(location, currentUrl) — signed query params in the initial URL are naturally dropped when the redirect target has its own query (or no query), matching AWS presigned-URL semantics.
• fetchPublicHttpsText keeps the single AbortController timeout live across fetch + body consumption via the reader loop (stalled.css test); maxBytes enforced both from Content-Length and streamed-byte accounting (large.css cancels the underlying stream).
• Redaction is applied at every diagnostic surface I could find:
  • classifyHttpFailure(status) no longer echoes statusText (that was the "503 statusText echoes signed URL back to log" trap — regression-guarded by html-error.mp4 test).
  • classifyDownloadFailure uses bounded generic strings ("Download failed due to a transient network error" / "Download failed while writing the local artifact"), so an Undici wrapper TypeError: fetch failed while requesting <signedUrl> cannot leak — verified by redacts signed URLs embedded in underlying fetch failures.
  • assertPublicHttpsUrl no longer echoes the URL on any of its three failure modes.
  • safeDownloadUrlIdentity publishes { urlFingerprint, host } — telemetry etagFingerprint is sha256(etag), not the raw ETag. Producer's defaultLogger.warn sites all pass { urlFingerprint, host } instead of the raw URL. The customer.css?X-Amz-Signature=super-secret stylesheet-log test explicitly confirms host is present and query/path are absent.

Other lenses (clean)

• Deferred parity: gh api search/code shows every downloadToTemp caller lives in a file touched by this PR (studioServer.ts, audioMixer.ts, videoFrameExtractor.ts, htmlCompiler.ts, plus tests + re-exports). No stale caller on the pre-refactor signature.
• Widened search: producer's separate fetch(href, …) in fetchExternalStylesheetCss was the only other unbounded fetch in this flow and it's now routed through fetchPublicHttpsText. CLI capture (packages/cli/src/capture/assetDownloader.ts) is a separate flow explicitly out of scope for this PR.
• Test discipline: expect(result.failures).toEqual([expect.objectContaining({ stage, owner, retryable })]) in the new audioMixer.test.ts case asserts semantics, not just presence. Range/checksum tests assert both the kind AND the rangeDisposition / locallyRetryable telemetry. feedback_test_asserts_guard_semantics_not_presence-style: fine.
• Pre-existing test comments preserved / adjacent invariants (log-format renames from "Remote media download failed for" → "Remote media download failed" are structural rather than message-shape regressions since callers now log { urlFingerprint, host, error }).

Verdict

Approving. The single boundary nit above is a follow-up hardening; publication correctness is preserved by the atomic linkSync no-clobber even if the lock invariant is briefly broken at the stale/timeout boundary.

— Review by Via

Review: fix(engine): validate remote download integrity @ 90adf48b

Reviewed by: 5 independent adversarial agents (cross-process locking, checksum integrity, SSRF/redaction, error classification/retry, test coverage)

SSRF and signed-URL redaction: clean

Every signed-URL leak vector I checked is closed:

• statusText dropped from error messages (the actual CDN leak vector)
• assertPublicHttpsUrl no longer echoes the URL in errors
• classifyDownloadFailure uses hardcoded messages, not raw error.message
• /* Inlined from ${href} */ replaced with generic comment
• Log messages use safeDownloadUrlIdentity (SHA-256 of origin+pathname, hostname only)
• Redirect validation runs per-hop in fetchWithValidatedRedirects with redirect: "manual" before every fetch()
• isBlockedHost covers IPv4-mapped IPv6 (::ffff:0:0/96), link-local, and all RFC 1918 ranges

Findings

1. Content-MD5 can reject content already verified correct by SHA-256

fetchToPartial (urlDownloader.ts)

const checksumMatches =
  expectedSha256 === null ||
  (expectedSha256.encoding === "base64"
    ? sha256Bytes.toString("base64") === expectedSha256.value
    : localSha256 === expectedSha256.value);
const contentMd5 = response.headers.get("content-md5")?.trim();
if (!checksumMatches) {
  throw checksumMismatchError(...);
}
if (contentMd5 && md5Base64 !== contentMd5) {
  throw checksumMismatchError("server", response.status, telemetry);
}

When a caller provides expectedSha256 (or the server sends Digest: sha-256=...) and it matches, the content is cryptographically verified correct. But the Content-MD5 check still runs unconditionally afterward. If the CDN serves a stale or per-part Content-MD5 (common with S3 multipart uploads, CDN cache rotations), verified-correct content is rejected with retryable: true. The retry loop re-fetches, same stale MD5, same rejection, exhausting the retry budget.

SHA-256 is strictly stronger than MD5. Once SHA-256 passes, MD5 is redundant. Suggested fix:

if (expectedSha256 === null && contentMd5 && md5Base64 !== contentMd5) {
  throw checksumMismatchError("server", response.status, telemetry);
}

This preserves Content-MD5 as a fallback when no SHA-256 verification is available.

2. Content-Range on 200 may regress non-compliant CDN downloads

classifyRangeDisposition (urlDownloader.ts)

return response.status === 200 && contentRange !== null ? "content_range_on_200" : "none";

Any non-none disposition triggers a range_protocol rejection. RFC 9110 section 14.4 forbids Content-Range on 200 responses without a Range request, but some S3-compatible stores (MinIO, older Ceph) and CDN proxies include Content-Range: bytes 0-N/N on full-object 200 responses. The old code accepted these silently. The new code rejects with retryable: true, causing 4 futile fetches (2 local retries x 2 upstream retries) before permanent failure.

This is the most likely regression from this PR in production. Worth considering: if the Content-Range on a 200 describes the full object (bytes 0-N/N where N+1 = Content-Length), treating it as "none" would preserve compatibility while still rejecting genuinely malformed cases.

3. race_reused inspection ENOENT becomes non-retryable

runDownloadAttempt (urlDownloader.ts)

if (outcome === "race_reused") {
  const inspection = await inspectExistingFile(localPath);  // no lock held

After linkSync returns EEXIST (another caller published first), the code inspects the winner's file without the cache lock. If a concurrent reuseOrInvalidateCachedFile (e.g., mixed-version process that published an error document) deletes the file between the EEXIST and inspectExistingFile, the ENOENT propagates through classifyDownloadFailure as { kind: "filesystem", retryable: false }. A transient race condition becomes a permanent failure.

The window is narrow (requires cross-process mixed-version timing), but the fix is simple: catch ENOENT in the race_reused path and fall through to a fresh download attempt.

4. Missing ENOENT guard on first lstatSync in reuseOrInvalidateCachedFile

if (!existsSync(localPath)) return false;
const before = lstatSync(localPath);   // <-- no try/catch for ENOENT

The subsequent inspectExistingFile and the second lstatSync(after) both handle ENOENT with catch (ENOENT) { continue }. The first lstatSync(before) does not, despite the same race window (file deleted between existsSync and lstatSync). The lock is held so this requires external intervention, but the asymmetry with the other two guards is a gap. Trivial fix: wrap in the same pattern.

Test coverage gaps

1. Content-MD5 validation untested. The MD5 comparison at lines 1591-1597 has no dedicated test. A test verifying that a correct-SHA256 + wrong-MD5 response still publishes (or doesn't, depending on the design decision above) would lock in the expected behavior.

2. Stale lock recovery (5-minute timeout) untested. acquireCachePathLock recovers stale locks based on statSync(lockPath).mtimeMs, but no test backdates a lock directory's mtime and verifies recovery. If the comparison is inverted (< instead of >), all downloads to that cache path block until timeout.

3. BOM prefix in error document detection untested. looksLikeRemoteErrorDocument strips \uFEFF via regex, but the existing HTML test uses leading whitespace, not a BOM. S3/CloudFront error pages can include BOMs.

Minor observations (not findings)

• assertPublicHttpsUrl is dropped from the producer re-export (packages/producer/src/utils/urlDownloader.ts). It remains on @hyperframes/engine. External consumers of the producer re-export path would need to update their import.
• cancelled maps to owner: "user" in downloadFailure() (audioMixer.ts). Semantically odd (system cancelled, not user), but functionally correct (retryable: false).
• The studioServer.ts call uses downloadToTemp(url, destDir, undefined, undefined, undefined, { onTelemetry }) — 3 positional undefineds. Works, but an options-object signature would be cleaner.

Verdict

Solid security and integrity hardening. The signed-URL redaction is thorough, the SSRF validation is correct per-hop, the linkSync atomic publication is a meaningful upgrade over renameSync, and the retry classification (locallyRetryable vs retryable) is carefully designed.

Findings 1-2 are the ones worth discussing before merge — the Content-MD5/SHA-256 ordering is a design question, and the Content-Range-on-200 regression could surface in production with non-compliant CDNs. Findings 3-4 are narrow concurrency gaps with simple fixes.

Focused delta re-review at 338a739e5 (90adf48b4 → current head).

The four earlier findings are closed in the implementation, not only in the tests:

• SHA-256 is now authoritative when available; Content-MD5 is used only as a fallback (urlDownloader.ts:907-919). This handles both caller-supplied and server SHA-256 without allowing a contradictory legacy MD5 header to reject already-verified bytes.
• A status-200 Content-Range is accepted only when it proves a full identity-encoded object: start 0, end total - 1, safe integers, and Content-Length === total (:671-693). The streamed byte-count check at :884-895 still rejects a short body.
• Both EEXIST winner-disappearance windows are converted to retryable filesystem failures (:957-969, :1017-1030), and the initial cache lstat now treats ENOENT symmetrically with the later read/stat checks (:1189-1216).
• Lock ownership is generation-stable and owner-specific. Release/reclaim consumes the exact owner marker and uses non-recursive removal, so a predecessor cannot recursively delete a successor lock (:290-328, :350-430).

The new tests falsify each repaired behavior, including conflicting SHA/MD5 headers, complete versus partial/encoded Content-Range responses, short bodies, BOM-prefixed error pages, stale-lock recovery, stale-to-successor replacement, predecessor release, and both ENOENT race windows. I ran the focused downloader suite locally: 79/79 passed. Formatting and lint on the two delta files are clean. At the exact head, all latest non-cancelled CI contexts are successful or intentionally skipped, including all eight required checks and the Windows lanes.

Via's earlier non-blocking stale-boundary observation remains a possible duplicate-work edge at the five-minute boundary; the new owner marker and atomic no-clobber publication keep it from becoming successor-lock deletion or artifact corruption.

Audited: The complete 90adf48b4 → 338a739e5 production delta in packages/engine/src/utils/urlDownloader.ts, the corresponding new test delta, exact-head required/full CI, and prior review findings.

Trusting: The nine unchanged files already covered by the previous full-head reviews; generated regression artifacts and unchanged pre-delta behavior were not re-audited in this focused pass.

Verdict: APPROVE
Reasoning: The focused delta closes every previously identified correctness gap with narrowly scoped code and falsifiable race/protocol tests; no blocking issue remains at the shipping head.

— Magi