fix(catalog): confine hosted preview asset downloads - #3843
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed at head 7069e303 in an isolated worktree. Additive to the CodeQL bot's note (the new alert is the same sink, relocated — see Notes).
Strengths
scripts/catalog-hosted-files.ts:51-53—assertCdnUrlruns oncurrentbefore everyfetch, including hop 0, andredirect: "manual"means no hop is ever followed by the runtime. Mutating the loop to assert only on hop 0 fails exactly the four redirect tests.catalog-hosted-files.ts:11— comparingurl.origin(not host or a prefix) gets scheme, host and port in one check. Probed at this head:https://user:pw@static.heygen.ai/a,https://static.heygen.ai:8443/aand a punycode lookalike are rejected before any request;https://STATIC.heygen.ai:443/anormalises and is allowed, which is correct.catalog-hosted-files.ts:16-39— declared and streamed bytes are both capped;Number(null) === 0means an absentContent-Lengthfalls through to the stream count rather than bypassing it. Dropping the stream cap makes the understated-length test never terminate; dropping the header check fails the declared-oversize test.catalog-hosted-files.ts:84-90+registry-target-paths.mjs:64-70—rootis the realpath andisContainedInrefuses any existing symlink component, so a committed symlink at the destination or in a parent directory pointing outside the copy is skipped without a request. Probed both at this head.
Important (non-blocking)
catalog-hosted-files.test.ts— two mutants survive the 12 tests: removing theusername/passwordrefusal, and replacing theorigincompare withprotocol + hostname(which would admit:8443). The behaviour is right at this head (probed above), but neither the credential nor the port arm is pinned. Two one-line initial-URL cases close it.catalog-hosted-files.test.ts:72-99— the streaming-cap test is backed by an infinitepull(). If the cap regresses, the test hangs rather than fails (my mutant had to be killed at 120 s). Enqueue a bounded number of chunks (three of 129 MiB is enough to cross the cap) so a regression reads as a red test.
Nits
- The 256 MiB cap is generous against a 154 KB maximum in the current 396 references. A cap near the largest plausible asset (tens of MiB) is a tighter resource bound for a
pull_requestrunner and costs nothing today; a comment naming the rationale either way would help the next reader. - This is now a third fetch policy alongside the engine's
BlockListand the media skill's redirect policy. Deliberately different (allowlist vs. public-only), and the skill ships outside the workspace, so I am not asking for a refactor — only noting the layering so a future host change is made in all three. fetchHostedAsseterror messages no longer include the offending URL (the old inline code did), which makes a red preview job harder to read; consider appendingurlto the CDN and HTTP-error messages.
Verified at this head
node --import tsx --test scripts/catalog-hosted-files.test.ts: 12/12.- Mutants: hop-0-only assertion (killed, 4 tests), drop stream cap (killed by non-termination), drop
Content-Lengthcheck (killed), hop limit 5 → 50 (killed by the loop test), drop credential check (survives), origin → protocol+hostname (survives). - Every hosted
urlin the repo'sregistry-item.jsonfiles (396) is onhttps://static.heygen.ai, matching the PR's count. - Required checks all green at
7069e303, includingTests on windows-latest;mergeable.
Notes
- CodeQL re-raised the network-to-file rule on the relocated
writeFileSync(catalog-hosted-files.ts:90). That is the same sink with the source now confined to one HTTPS origin, size-capped and path-contained; by design rather than a false positive, and the classification is the maintainers' call. No alert is touched by this review. - The 120 s deadline is one
AbortSignal.timeoutshared across all hops and the body read, so it bounds the whole download rather than only the headers (read, not exercised — the mockedResponsecannot observe it).
Verdict: APPROVE
Reasoning: The boundary is exact-origin per hop, size-capped on both declared and actual bytes, and path-contained on the realpath, and every one of those arms is pinned by a test that a mutant flips; the two surviving mutants cover behaviour that is correct at this head but untested.
— Miga (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Re-review at head f0ba3e93b6a8978d5422fa13d2d92d81886ea69f. Interdiff from 7069e303 (which I approved): scripts/catalog-hosted-files.test.ts +20/−11 and one comment line in scripts/catalog-hosted-files.ts. No runtime change.
What the new head fixes from my previous review
catalog-hosted-files.test.ts:61-75— the initial-URL rejection is now a table over127.0.0.1,user:pw@static.heygen.aiandstatic.heygen.ai:8443, each asserting zerofetchcalls. Verified: the two mutants that survived last time are now both killed — dropping theurl.username || url.passwordcheck fails test 7; replacing the exact-origincomparison with protocol+hostname fails test 8.catalog-hosted-files.test.ts:80-91— the streaming-cap fixture closes after three 129 MiB chunks. Verified: with the streamed-bytes cap raised toNumber.MAX_SAFE_INTEGER, the test now fails in under two seconds with "Missing expected rejection" instead of hanging the runner.catalog-hosted-files.ts:8— the 256 MiB rationale (match the media-freeze ceiling) is now in the source.- 14/14 tests pass locally at this head.
Why COMMENT rather than APPROVE right now
Required check Render on windows-latest is red at f0ba3e93. I read the job log: the canary render fails because the downloaded chrome-headless-shell.exe --version probe was killed with SIGKILL, ETIMEDOUT on the runner. This PR touches only scripts/ and does not reach that path; the Windows lane is green on main at a7e2e385, d393541c and 962c9540. So the failure is environmental, but I will not approve over a red required check. Test is still pending on this head as well. Once the Windows job is rerun green and Test passes at this same commit, I will post the approval pointing back here; nothing else is outstanding.
Verdict: COMMENT
Reasoning: Code and tests are ready (both previously surviving mutants now killed, cap test terminates), but a required check is red on this head for an unrelated runner failure; approval follows a green rerun at f0ba3e93.
— Miga (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Approving at head f0ba3e93b6a8978d5422fa13d2d92d81886ea69f. Nothing in the code changed since my comment review at this same head (#3843 (review)), which holds the verification: 14/14 locally, the credential and non-default-port mutants now killed, and the streaming-cap test failing promptly rather than hanging when the cap is removed.
The one thing that was outstanding is now resolved: every required check, including Render on windows-latest after its rerun and Test, is green at f0ba3e93.
Verdict: APPROVE
Reasoning: Same verified head as the comment review; the only blocker was a red required check caused by a runner-side Chrome timeout, and it is green on rerun. Merging is Miguel's call.
— Miga (pr-review)
Catalog previews run contributor manifests on pull requests. Previously a hosted asset URL could redirect from HTTPS to a private runner endpoint, and its entire response was buffered without a limit. Confine initial URLs and every redirect to
https://static.heygen.ai, reject URL credentials, follow at most five redirects, and cap both declared and actual streamed bytes at 256 MiB with a 120-second request deadline. Failed downloads leave an existing destination untouched.The extracted helper keeps manifest destination containment and uses the same real project root for validation and writing. Byte contents and manifest paths remain unchanged; empty successful responses retain existing behavior. The CDN restriction is deliberate: all 396 current hosted-file references use 23 distinct URLs on that origin. Live HEAD checks for all 23 succeeded, with the largest asset 154,010 bytes, well below the cap. Future registry assets on another origin require an explicit policy change.
Addresses the concrete fetch-boundary defect behind #903 and the connected persistence flow #901. Neither alert is dismissed by this PR; final scanner state and any by-design classification require independent verification. DNS for the fixed CDN remains trusted.
Validation: full workspace build, 213 script tests, 86 catalog tests, and 12 focused download regressions pass. The original fetch function fails the new initial-origin regression. Tests cover private/downgrade/lookalike redirects, exact bytes, traversal rejection, redirect limits, declared and actual size caps, stream/HTTP failures, and preservation of existing output. Lint/format, strict repository typecheck and signed hooks pass. Fallow passes its changed-code gate with four pre-existing findings excluded; no suppression added.