fix(media): validate every download redirect target - #3841
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed at head a40c89f7 (1 commit on 962c9540/main; 8 files, +279/−37). No prior reviews. Boundary audit of the shared policy, not just the five flagged sites.
Strengths
media-fetch.mjs:57-69— redirects are followed manually (redirect: "manual"), andisPublicMediaUrlruns on the initial target and on every hop before that hop is requested; a 3xx body is cancelled before moving on; relativeLocationis resolved against the current hop; the caller'ssignalis forwarded per hop. Cap is 5 redirects (6 requests), pinned bybounds redirect loopsassertingcalls === 6.media-fetch.mjs:37-55— the host check is done on the WHATWG-parsedhostname, so decimal/octal/hex IPv4 (2130706433,0x7f000001,127.1) are canonicalised beforeBlockList, userinfo (http://good@127.0.0.1) cannot smuggle a host, case and trailing dot are normalised, andBlockList.checktreats IPv4-mapped IPv6 as IPv4 so[::ffff:a00:1]hits the10/8rule. Any non-http(s)hop (data:,file:,javascript:) fails the scheme test and is refused before a request. All of this is in the first test's vectors, and I re-ran them.freeze.mjs:60—isDirectMediaUrlnow delegates to the same policy instead of the old hand-rolled regex, so the direct-ingest gate and the download gate cannot drift apart.heygen-video-provider.test.mjs— keeping the provider URL public and swapping only the transport is the right way to keep real-bytes fixtures under a policy that rejects loopback.
Verified
- Baseline failure: with the four callers reverted to
mainand the new helper + tests kept, the five entrypoint tests fail (freeze,heygen audio,tts mp3,tts wav,favicon) and the four helper tests pass — so the regressions bite the callers, not the helper. Restored:media-fetch.test.mjs+heygen-video-provider.test.mjs20/20. - Caller enumeration (
grep fetch(|undici|http.get|https.get|got(|axiosoverskills/media-use, non-test): the remaining barefetchsites areheygen.mjs:117(authenticated API toHEYGEN_BASE, explicit non-goal),telemetry.mjs:172(PostHog batch, not media), andlogo-provider.mjs:115— see below. - CI at this head:
Test: skills, manifest sync, project-native lint + mirror, CodeQL analyze all green; Build/Test/Typecheck/Windows lanes are path-skipped for a skills-only diff (skipped, not failed).
Important
logo-provider.mjs:114-117—urlExists()still issues a barefetch(url, { method: "HEAD" })with default redirect following. It is called on${SIMPLE_ICONS_CDN}/${slug}.svg(:150-153) andhttps://github.com/${org}.png?size=460(:176-178). Both initial targets are fixed public hosts, and onlyres.okis consumed, so nothing private is persisted — but a redirect from either host to a private/metadata address would be followed and requested, which is the class this PR closes elsewhere in the same file (fetchJson,faviconSearch). Route it through the policy: givefetchMediaamethodpassthrough (fetchImpl(current, { method, signal, redirect: "manual" })) and callfetchMedia(url, { method: "HEAD", signal }). One-line change plus one test; not a blocker because the probe result is boolean and the hosts are constants, but it is the one outbound request in these flows that bypasses the shared helper.
Non-blocking
- nit
media-fetch.mjs:40— anhttps:→http:downgrade on redirect is allowed (both schemes pass). Consistent with "public HTTP stays supported", but worth one line in the header comment so nobody reads the policy as scheme-preserving. - nit
media-fetch.mjs:26-35— not in the v6 list:2002::/16(6to4, embeds an IPv4 that can be private),64:ff9b::/96(NAT64), IPv4-compatible::a.b.c.d. None are routable to private space on a normal host, so this is completeness, not a gap; adding the first two is two lines. - nit
media-fetch.mjs:63-65— a 3xx with noLocationis returned to the caller as-is (callers checkres.ok, so it becomes an HTTP error). Fine; a comment saying so would save the next reader a question. - The PR body's non-goals (DNS trusted, no rebinding protection, MEDIA_EXT not enforced on provider results) match the code and the header comment on
media-fetch.mjs:1-3. No alert touched by me.
Verdict: APPROVE
Reasoning: One shared policy, validated before each request on the initial URL and every redirect hop, with the five entrypoint regressions proven to fail on the original callers; the one remaining bypass is a boolean HEAD probe to constant public hosts, filed as important with a one-line fix rather than a blocker.
— Miga (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed at head 6862e916 (2 commits on 962c9540/main). Interdiff from a40c89f7 (my APPROVE above stands as the full read): media-fetch.mjs:58,63 gains a method passthrough (GET default) and the header comment now states that public HTTPS→HTTP redirects are allowed; logo-provider.mjs:115 routes urlExists() through fetchMedia(url, { method: "HEAD", signal }); two entrypoint tests (simpleIconsSearch, githubAvatarSearch) reject a private HEAD redirect after exactly one request; the relative-redirect test pins that HEAD and the signal survive the hop. Manifest hash updated.
Re-verified at this head
media-fetch.test.mjs+logo-provider.test.mjs+heygen-video-provider.test.mjs: 36/36 locally.- Baseline check: with
logo-provider.mjsandmedia-fetch.mjsreverted toa40c89f7, the two new HEAD-probe tests fail and the other 14 pass — the regressions bite the change, not the harness. - Caller enumeration re-run: no bare
fetchremains on a media/provider URL inskills/media-use; the two left are the authenticated API transport (heygen.mjs:117) and telemetry (telemetry.mjs:172), both explicit non-goals. - All required checks green at this head (Build, Test, Typecheck, runtime contract, Windows render, regression, title); skills lanes and CodeQL analyze green.
Scope call on the IPv6 transition ranges (2002::/16, 64:ff9b::/96): keeping them out is acceptable — neither is routable to private space on an ordinary host, the policy stays "ordinary private/reserved literal hosts", and the DNS-trust limitation remains stated in the header. Nit only, as before.
Verdict: APPROVE
Reasoning: The one outbound request in these flows that bypassed the shared policy now goes through it, proven by tests that fail on the previous head; CI green at the exact head; no alert touched.
— Miga (pr-review)
Media downloads previously followed redirects automatically. A public media or provider URL could redirect to a private/metadata endpoint and persist that response locally. Validate the initial URL and every redirect through one shared HTTP(S) helper before any request, with a five-redirect limit and cancellation of redirect bodies.
Applies to the five related code-scanning flows: #718 freeze, #732 HeyGen audio, #733 TTS WAV/transcode, #734 TTS raw MP3, and #742 favicon. Also covers generated video and LUT downloads through their existing freeze caller. These alert IDs remain open pending independent post-fix classification; this PR does not suppress the intentional download capability.
Public HTTP and HTTPS, relative redirects, cancellation, provider output bytes, the existing freeze size cap, and caller output/transcode behavior are preserved. LUT
.cubedownloads remain supported, so the direct-ingest media extension filter is not imposed on provider results. Literal private/reserved IPs, IPv4 aliases/mapped IPv6, and local/internal hostnames are blocked. DNS resolution remains trusted: this does not claim DNS-rebinding protection or introduce DNS pinning. Authenticated API requests are unchanged.Validation: full workspace build passed; full skills suite 619 passed / 2 skipped. Nine focused network tests pass, including all five actual entrypoints rejecting public-to-private redirects before saving/transcoding; those five tests fail against the original code. Existing generated-video HTTP fixtures still transfer real bytes via a test-only public-URL-to-local-server transport. Lint, formatting, skill manifest, skill mirror, Fallow and signed commit hooks pass.